Re: [PATCH] PCI: avoid bogus calls to get_pseg()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.08.2022 14:13, Andrew Cooper wrote:
> On 09/08/2022 16:50, Jan Beulich wrote:
>> When passed -1, the function (taking a u16) will look for segment
>> 0xffff, which might exist. If it exists, we may find (return) the wrong
>> device.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> An alternative would be to declare that both functions cannot be called
>> with "wildcards" anymore. The last such use went away with f591755823a7
>> ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment
>> failed") afaict.
> 
> The way wildcards were used before were always bogus IMO.
> 
> I suggest we take this opportunity to remove the ability to re-introduce
> that anti-pattern.

Okay, will do that in v2. Rahul - this means there's no point anymore
sending a v2 of your fix, as the bug will disappear as a side effect.
I'll add you as the reporter of that bug.

>> Each time I look at this pair of functions I wonder why we have two
>> copies of almost the same code (with a curious difference of only one
>> having ASSERT(pcidevs_locked())). Any opinions on deleting either one,
>> subsuming its functionality into the other one by allowing the domain
>> pointer to be NULL to signify "any"?
> 
> I'm in two minds about this.  Because they are the same, they ought to
> be deduped.
> 
> Except they're both insane and both want changing to a less silly
> datastructure, at which point they will be different.
> 
> It is a total waste to do an O(n) loop over all PCI devices in the
> system checking for equality to single device (and in the domain case,
> assignment to the domain).  The domain variant should loop over the pci
> devices in that domain, because it is guaranteed to be a shorter list
> than all devices.

With the "wildcard" support gone, that's going to be sensible, yes,
and I'll switch to that. Except for hwdom, where I mean to stick to
the per-segment all-devices lists, as on multi-segment systems these
are very likely the shorter lists, while on single-segment systems
the sole all-devices list likely isn't much longer than the list of
hwdom's devices (the delta being all devices [intended to be] passed
through plus all hidden devices). At this point I'm not sure whether
it would be worth further special-casing the single-segment case,
even if that's (on x86) the applicable one on the vast majority of
systems.

> The global lookup probably wants to investigate a more efficient
> datastructure because I bet this is a hotpath.

I don't think it's a hot path, but it can certainly be improved. Yet
that's future work, nothing to be done right here. But I'm inferring
you agree in this regard by saying "investigate".

Jan