Re: [PATCH v3] vfio/mdev: Check globally for duplicate devices

[Alex Williamson]

On Thu, 17 May 2018 15:37:37 -0600
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:

> On Fri, 18 May 2018 01:56:50 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> 
> > On 5/17/2018 9:50 PM, Alex Williamson wrote:  
> > > On Thu, 17 May 2018 21:25:22 +0530
> > > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> > >     
> > >> On 5/17/2018 1:39 PM, Cornelia Huck wrote:    
> > >>> On Wed, 16 May 2018 21:30:19 -0600
> > >>> Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
> > >>>       
> > >>>> When we create an mdev device, we check for duplicates against the
> > >>>> parent device and return -EEXIST if found, but the mdev device
> > >>>> namespace is global since we'll link all devices from the bus.  We do
> > >>>> catch this later in sysfs_do_create_link_sd() to return -EEXIST, but
> > >>>> with it comes a kernel warning and stack trace for trying to create
> > >>>> duplicate sysfs links, which makes it an undesirable response.
> > >>>>
> > >>>> Therefore we should really be looking for duplicates across all mdev
> > >>>> parent devices, or as implemented here, against our mdev device list.
> > >>>> Using mdev_list to prevent duplicates means that we can remove
> > >>>> mdev_parent.lock, but in order not to serialize mdev device creation
> > >>>> and removal globally, we add mdev_device.active which allows UUIDs to
> > >>>> be reserved such that we can drop the mdev_list_lock before the mdev
> > >>>> device is fully in place.
> > >>>>
> > >>>> NB. there was never intended to be any serialization guarantee
> > >>>> provided by the mdev core with respect to creation and removal of mdev
> > >>>> devices, mdev_parent.lock provided this only as a side-effect of the
> > >>>> implementation for locking the namespace per parent.  That
> > >>>> serialization is now removed.      
> > >>>       
> > >>
> > >> mdev_parent.lock is to serialize create and remove of that mdev device,
> > >> that handles race condition that Cornelia mentioned below.    
> > > 
> > > Previously it was stated:
> > > 
> > > On Thu, 17 May 2018 01:01:40 +0530
> > > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:    
> > >> Here lock is not for create/remove routines of vendor driver, its about
> > >> mdev device creation and device registration, which is a common code
> > >> path, and so is part of mdev core module.    
> > > 
> > > So the race condition was handled previously, but as a side-effect of
> > > protecting the namespace, aiui.  I'm trying to state above that the
> > > serialization of create/remove was never intended as a guarantee
> > > provided to mdev vendor drivers.  I don't see that there's a need to
> > > protect "mdev device creation and device registration" beyond conflicts
> > > in the UUID namespace, which is done here.  Are there others?
> > >     
> > 
> > Sorry not being elaborative in my earlier response to
> >   
> > > If we can
> > > show that vendor drivers handle the create/remove paths themselves,
> > > perhaps we can refine the locking granularity.    
> > 
> > mdev_device_create() function does :
> > - create mdev device
> > - register device
> > - call vendor driver->create
> > - create sysfs files.
> > 
> > mdev_device_remove() removes sysfs files, unregister device and delete
> > device.
> > 
> > There is common code in mdev_device_create() and mdev_device_remove()
> > independent of what vendor driver does in its create()/remove()
> > callback. Moving this code to each vendor driver to handle create/remove
> > themselves doesn't make sense to me.  
> 
> I don't see where anyone is suggesting that, I'm not.
>  
> > mdev_parent.lock here does take care of race conditions that could occur
> > during mdev device creation and deletion in this common code path.  
> 
> Exactly what races in the common code path is mdev_parent.lock
> preventing?  mdev_device_create() calls:
> 
> device_register()
> mdev_device_create_ops()
>   parent->ops->create()
>   sysfs_create_groups()
> mdev_create_sysfs_files()
>   sysfs_create_files()
>   sysfs_create_link()
>   sysfs_create_link()
> 
> mdev_parent.lock is certainly not serializing all calls across the
> entire kernel to device_register and sysfs_create_{groups,files,link}
> so what is it protecting other than serializing parent->ops->create()?
> Locks protect data, not code.  The data we're protecting is the shared
> mdev_list, there is no shared data once mdev_device_create() has its
> mdev_device with uuid reservation placed into that list.
> 
> > What is the urge to remove mdev_parent.lock if that handles all race
> > conditions without bothering user to handle -EAGAIN?  
> 
> Can you say why -EAGAIN is undesirable?  Note that the user is only
> going to see this error if they attempt to remove the device in the
> minuscule gap between the sysfs remove file being created and the
> completion of the write to the create sysfs file.  It seems like you're
> asking that I decrease the locking granularity, but not too much
> because mdev_parent.lock protects "things".  If the -EAGAIN is really
> so terrible, we can avoid it by spinning until the mdev_device is
> either not found in the list or becomes active, we don't need
> mdev_parent.lock to solve that, but I don't think that's the best
> solution and there's no concrete statement to back -EAGAIN being a
> problem.  Thanks,

Testing whether -EAGAIN is bad, I race the following scripts:

# cat remove-it.sh 
#!/bin/sh

UUID=802d71a3-560d-4316-999a-4c2033b7fbb2
DEV=/sys/devices/virtual/mtty/mtty/$UUID
REMOVE=$DEV/remove
CNT=0
ERR=0

while true; do
	if [ -e $REMOVE ]; then
		echo 1 > $REMOVE
		RET=$?
		if [ $RET -ne 0 ]; then
			echo "Remove returned $RET"
			ERR=$(( $ERR + 1 ))
		fi
		CNT=$(( $CNT + 1 ))
		if [ $(( $CNT % 10000 )) -eq 0 ]; then
			echo "$CNT/$ERR"
		fi
	fi
done

# cat create-it.sh 
#!/bin/sh

UUID=802d71a3-560d-4316-999a-4c2033b7fbb2
DEV=/sys/devices/virtual/mtty/mtty/$UUID
CREATE=/sys/class/mdev_bus/mtty/mdev_supported_types/mtty-1/create
CNT=0

while true; do	
	if [ ! -d $DEV ]; then
		echo $UUID > $CREATE
		RET=$?
		if [ $RET -ne 0 ]; then
			echo "Create returned $RET"
		fi
		CNT=$(( $CNT + 1 ))
		if [ $(( $CNT % 10000 )) -eq 0 ]; then
			echo $CNT
		fi		
	fi
done

After 3 million iterations, this targeted testing is only able to hit
the gap 0.0032% of the time and the sum badness is:

./remove-it.sh: line 11: echo: write error: Resource temporarily unavailable

Of course if I have one thread of execution that does create then
remove, it's impossible to hit this case, so the only code that can hit
this is unsynchronized user code stepping on each other and -EAGAIN
seems like a completely reasonable and appropriate error, imo.  Thanks,

Alex