Message-ID: <20180226195131.71690e9a.cohuck@redhat.com>
Date:   Mon, 26 Feb 2018 19:51:31 +0100
From:   Cornelia Huck <cohuck@...hat.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     kwankhede@...dia.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, cordius.wu@...wei.com,
        eskultet@...hat.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH RFC] vfio/mdev: delay uevent after initialization
 complete

On Wed, 14 Feb 2018 18:20:35 +0100
Cornelia Huck <cohuck@...hat.com> wrote:

Ping. Does anyone have an opinion on how to proceed?

> [cc:ing Greg for his opinion on this; retaining quoting for context]
> 
> On Tue, 13 Feb 2018 17:15:02 -0700
> Alex Williamson <alex.williamson@...hat.com> wrote:
> 
> > On Tue, 13 Feb 2018 14:09:01 +0100
> > Cornelia Huck <cohuck@...hat.com> wrote:
> >   
> > > On Mon, 12 Feb 2018 14:20:57 -0700
> > > Alex Williamson <alex.williamson@...hat.com> wrote:
> > >     
> > > > On Fri,  9 Feb 2018 11:27:16 +0100
> > > > Cornelia Huck <cohuck@...hat.com> wrote:
> > > >       
> > > > > The registration code first registers the mdev device, and then
> > > > > proceeds to populate sysfs. An userspace application that listens
> > > > > for the ADD uevent is therefore likely to look for sysfs entries
> > > > > that have not yet been created.
> > > > > 
> > > > > The canonical way to fix this is to use attribute groups that are
> > > > > registered by the driver core before it sends the ADD uevent; I
> > > > > unfortunately did not find a way to make this work in this case,
> > > > > though.
> > > > > 
> > > > > An alternative approach is to suppress uevents before we register
> > > > > with the core and generate the ADD uevent ourselves after the
> > > > > sysfs infrastructure is in place.
> > > > > 
> > > > > Signed-off-by: Cornelia Huck <cohuck@...hat.com>
> > > > > ---
> > > > > 
> > > > > This feels like a band-aid, but I can't figure out how to handle creating
> > > > > attribute groups when there's a callback in the parent involved.
> > > > > 
> > > > > This should address the issue with libvirt's processing of mdevs raised in
> > > > > https://www.redhat.com/archives/libvir-list/2018-February/msg00023.html
> > > > > - although libvirt will still need to deal with older kernels, of course.
> > > > > 
> > > > > Best to consider this an untested patch :)        
> > > > 
> > > > I agree, this feels like a band-aide.  If every device in the kernel
> > > > needs to suppress udev events until until some key component is added,
> > > > that suggests that either udev is broken in general or not being used
> > > > as intended.        
> > > 
> > > I think udev is working exactly as designed - it's more a problem of
> > > when the kernel is sending what kind of notification to userspace, and
> > > the particular issue here is how the code sending the event (driver
> > > core) and the code assembling part of the user interface (mdev)
> > > interact.
> > >     
> > > > Zongyong submitted a different proposal to fix this
> > > > here[1].  That proposal seems a bit more sound and has precedence
> > > > elsewhere in the kernel.  What do you think of that approach?  We
> > > > don't need both afaict.  Thanks,
> > > > 
> > > > Alex
> > > > 
> > > > [1]https://patchwork.kernel.org/patch/10196197/      
> > > 
> > > Zongyong's patch is sending an additional CHANGE uevent, and I agree
> > > that doing both does not make sense. However, I think the CHANGE uevent
> > > is not quite suitable in this case, and delaying the ADD uevent is
> > > better.
> > > 
> > > [Warning, the following may be a bit rambling.]
> > > 
> > > The Linux driver model works under the assumption that any device is
> > > represented as an in-kernel object that exposes information and knobs
> > > through sysfs. As long as the device exists, userspace can poke at the
> > > sysfs entries and retrieve information or configure things.
> > > 
> > > The idea of the 'ADD' uevent is basically to let userspace know that
> > > there is now a new device with its related sysfs entries, and it may
> > > look at it and configure it. IOW, if I (as a userspace application) get
> > > the ADD uevent, I expect to be able to look at the device's sysfs
> > > entries and find all the files/directories that are usually there,
> > > without having to wait.
> > > 
> > > This expectation is broken if a device is first registered with the
> > > driver core (generating the ADD uevent) and the driver adds sysfs
> > > attributes later. To fix this, the driver core added a way to specify
> > > default attribute groups for the device, which are registered by the
> > > driver core itself before it generates the ADD uevent. Unfortunately, I
> > > did not see a way to do this here (which does not mean there isn't).
> > > The alternative was to prevent the driver core from sending the ADD
> > > uevent and do it from the mdev code when it was ready.
> > > 
> > > The 'CHANGE' uevent, on the other hand, tells userspace that something
> > > has changed for the device (that already existed). I (as a userspace
> > > application) would expect to see it if, for example, the information
> > > exposed via sysfs has changed, or maybe even if new, optional, entries
> > > have appeared and I might want to rescan. With Zongyong's patch,
> > > userspace gets the CHANGE uevent for something that was already
> > > expected to be there, and is now _really_ there. It does give userspace
> > > an indication that it can now work with the device (which certainly
> > > improves things), but I would prefer to get rid of the too-early uevent
> > > completely so that userspace does not get notified at all before the
> > > device is completely present in sysfs.
> > > 
> > > So, in short, my patch does 'don't tell userspace until we're really
> > > done', and Zongyong's patch does 'tell userspace again when we're
> > > really done'.    
> > 
> > This all sounds reasonable, but don't we have this synchronization
> > problem _everywhere_?  I apologize for referencing this bug because it's
> > not public (https://bugzilla.redhat.com/show_bug.cgi?id=1376907) but
> > the gist of it is that soft-unplugging PCI devices using the remove
> > entry in sysfs and re-adding with rescan sysfs entry results in libvirt
> > seeing the ADD uevent before the PCI config attribute is created and it
> > balks on the device.   
> 
> Yikes. So it seems that _any_ PCI device is incomplete when the ADD
> uevent is sent? Or does this just apply to an unfortunately large
> number of drivers?
> 
> > So at the PCI core we have this same issue and
> > developers are saying that there's no guarantee that sysfs entries
> > won't be added and removed at any time in the lifecycle of the device
> > and it's not the kernel's responsibility to provide that
> > synchronization.   
> 
> I think this is mixing up two things:
> - sysfs entries that are there by default. These not being in place
>   when the ADD uevent is sent sounds broken.
> - Optional sysfs entries that might change. There may be a case for
>   those to be added/removed later on (probably paired with a CHANGE
>   uevent.)
> 
> > So what are we to do?  If this issue exists in a far
> > more predominant device subsystem than mdev, do we need to reset
> > developer expectations about what sort of synchronization, if any, the
> > kernel is intending to provide?  Should we agree and document some best
> > practices around ordering of (or suppressing of) uevents so that we can
> > consistently cleanup subsystems, or define that such ordering is
> > officially not guaranteed?  Thanks,
> > 
> > Alex  
> 
> I'd like to clarify expectations here as well. My understanding has
> been what I outlined above, and what I think allows userspace to rely
> on asynchronous notifications without having to resort to polling,
> waiting, etc.

Powered by blists - more mailing lists