On Tue, Feb 22, 2022 at 10:58:37AM +0000, Robin Murphy wrote:
On 2022-02-21 23:48, Jason Gunthorpe wrote:
On Mon, Feb 21, 2022 at 08:43:33PM +0000, Robin Murphy wrote:
On 2022-02-19 07:32, Christoph Hellwig wrote:
So we are back to the callback madness instead of the nice and simple
flag? Sigh.
TBH, I *think* this part could be a fair bit simpler. It looks like this
whole callback mess is effectively just to decrement
group->owner_cnt, but
Right, the new callback is because of Greg's push to put all the work
into the existing bus callback. Having symetrical callbacks is
cleaner.
I'll continue to disagree that having tons more code purely for the sake of
it is cleaner. The high-level requirements are fundamentally asymmetrical -
ownership has to be actively claimed by the bus code at a point during probe
where it can block probing if necessary, but it can be released anywhere at
all during remove since that cannot fail. I don't personally see the value
in a bunch of code bloat for no reason other than trying to pretend that an
asymmetrical thing isn't.
Then we should put this in the share core code like most of us want.
If we are doing this distorted thing then it may as well make some
kind of self consistent sense with a configure/unconfigure op pair.
group->owner? Walking the list would only have to be done for *releasing*
ownership and I'm pretty sure all the races there are benign - only
probe/remove of the driver (or DMA API token) matching a current non-NULL
owner matter; if two removes race, the first might end up releasing
ownership "early", but the second is waiting to do that anyway so it's OK;
if a remove races with a probe, the remove may end up leaving the owner set,
but the probe is waiting to do that anyway so it's OK.
With a lockless algorithm the race is probably wrongly releasing an
ownership that probe just set in the multi-device group case.
Still not sure I see what you are thinking though..
How did we get from adding a few simple lines to dd.c into building
some complex lockless algorithm and hoping we did it right?
It has to be s It should be pretty straightforward for
iommu_bus_notifier to clear group->owner automatically upon an
unbind of the matching driver when it's no longer bound to any other
devices in the group either.
That not_bound/unbind notifier isn't currently triggred during
necessary failure paths of really_probe().
Eh? Just look at the context of patch #2, let alone the rest of the
function, and tell me how, if we can't rely on BUS_NOTIFY_DRIVER_NOT_BOUND,
calling .dma_cleanup *from the exact same place* is somehow more reliable?
Yeah, OK
AFAICS, a notifier handling both BUS_NOTIFY_UNBOUND_DRIVER and
BUS_NOTIFY_DRIVER_NOT_BOUND would be directly equivalent to the callers of
.dma_cleanup here.
Yes, but why hide this in a notifier, it is still spaghetti
use-case) then it should be up to VFIO to decide when it's finally
finished with the whole group, rather than pretending we can keep
track of nested ownership claims from inside the API.
What nesting?
The current implementation of iommu_group_claim_dma_owner() allows owner_cnt
to increase beyond 1, and correspondingly requires
iommu_group_release_dma_owner() to be called the same number of times. It
doesn't appear that VFIO needs that, and I'm not sure I'd trust any other
potential users to get it right either.
That isn't for "nesting" it is keeping track of multi-device
groups. Each count represents a device, not a nest.
FWIW I have some ideas for re-converging .dma_configure in future
which I think should probably be able to subsume this into a
completely generic common path, given a common flag.
This would be great!
Indeed, so if we're enthusiastic about future cleanup that necessitates a
generic flag, why not make the flag generic to start with?
Maybe when someone has patches to delete the bus ops completely they
can convince Greg. The good news is that it isn't much work to flip
the flag, Lu has already done it 3 times in the previous versions..
It has already been 8 weeks on this point, lets just move on please.