Re: [Qemu-devel] [PATCH v7 1/4] vfio: Mediated device Core driver

[Alex Williamson]

On Tue, 20 Sep 2016 01:39:19 +0530
Kirti Wankhede <address@hidden> wrote:

> On 9/19/2016 11:41 PM, Alex Williamson wrote:
> > On Mon, 19 Sep 2016 22:59:34 +0530
> > Kirti Wankhede <address@hidden> wrote:
> >   
> >> On 9/12/2016 9:23 PM, Alex Williamson wrote:  
> >>> On Mon, 12 Sep 2016 13:19:11 +0530
> >>> Kirti Wankhede <address@hidden> wrote:
> >>>     
> >>>> On 9/12/2016 10:40 AM, Jike Song wrote:    
> >>>>> On 09/10/2016 03:55 AM, Kirti Wankhede wrote:      
> >>>>>> On 9/10/2016 12:12 AM, Alex Williamson wrote:      
> >>>>>>> On Fri, 9 Sep 2016 23:18:45 +0530
> >>>>>>> Kirti Wankhede <address@hidden> wrote:
> >>>>>>>      
> >>>>>>>>>> +
> >>>>>>>>>> +static struct parent_device *mdev_get_parent_from_dev(struct 
> >>>>>>>>>> device *dev)
> >>>>>>>>>> +{
> >>>>>>>>>> +  struct parent_device *parent;
> >>>>>>>>>> +
> >>>>>>>>>> +  mutex_lock(&parent_list_lock);
> >>>>>>>>>> +  parent = mdev_get_parent(__find_parent_device(dev));
> >>>>>>>>>> +  mutex_unlock(&parent_list_lock);
> >>>>>>>>>> +
> >>>>>>>>>> +  return parent;
> >>>>>>>>>> +}        
> >>>>>>>>>
> >>>>>>>>> As we have demonstrated, all these refs and locks and release 
> >>>>>>>>> workqueue are not necessary,
> >>>>>>>>> as long as you have an independent device associated with the mdev 
> >>>>>>>>> host device
> >>>>>>>>> ("parent" device here).
> >>>>>>>>>        
> >>>>>>>>
> >>>>>>>> I don't think every lock will go away with that. This also changes 
> >>>>>>>> how
> >>>>>>>> mdev devices entries are created in sysfs. It adds an extra 
> >>>>>>>> directory.      
> >>>>>>>
> >>>>>>> Exposing the parent-child relationship through sysfs is a desirable
> >>>>>>> feature, so I'm not sure how this is a negative.  This part of Jike's
> >>>>>>> conversion was a big improvement, I thought.  Thanks,
> >>>>>>>      
> >>>>>>
> >>>>>> Jike's suggestion is to introduced a fake device over parent device 
> >>>>>> i.e.
> >>>>>> mdev-host, and then all mdev devices are children of 'mdev-host' not
> >>>>>> children of real parent.
> >>>>>>      
> >>>>>
> >>>>> It really depends on how you define 'real parent' :)
> >>>>>
> >>>>> With a physical-host-mdev hierarchy, the parent of mdev devices is the 
> >>>>> host
> >>>>> device, the parent of host device is the physical device. e.g.
> >>>>>
> >>>>>         pdev            mdev_host       mdev_device
> >>>>>         dev<------------dev<------------dev
> >>>>>               parent          parent
> >>>>>
> >>>>>         Figure 1: device hierarchy
> >>>>>       
> >>>>
> >>>> Right, mdev-host device doesn't represent physical device nor any mdev
> >>>> device. Then what is the need of such device?    
> >>>
> >>> Is there anything implicitly wrong with using a device node to host the
> >>> mdev child devices?  Is the argument against it only that it's
> >>> unnecessary?  Can we make use of the device-core parent/child
> >>> dependencies as Jike has done w/o that extra node?
> >>>    
> >>
> >> I do feel that mdev core module would get simplified with the new sysfs
> >> interface without having extra node.  
> > 
> > Can you provide an example of why that is?
> >    
> 
> 'online' or earlier 'start'/'stop' interface is removed and would add
> open() and close() callbacks. ref count from struct mdev_device and
> mdev_get_device()/mdev_put_device() were added for this interface, these
> would go away.
> Using device-core parent/child dependencies between physical device and
> mdev device, other functions would get simplified.

Yes, we've refined the interface over time and I'm glad that you're
incorporating the device-core simplifications, but this doesn't really
argue for or against the extra device node.
 
> >>>>>> For example, directory structure we have now is:
> >>>>>> /sys/bus/pci/devices/0000\:85\:00.0/<mdev_device>
> >>>>>>
> >>>>>> mdev devices are in real parents directory.
> >>>>>>
> >>>>>> By introducing fake device it would be:
> >>>>>> /sys/bus/pci/devices/0000\:85\:00.0/mdev-host/<mdev_device>
> >>>>>>
> >>>>>> mdev devices are in fake device's directory.
> >>>>>>      
> >>>>>
> >>>>> Yes, this is the wanted directory.
> >>>>>       
> >>>>
> >>>> I don't think so.    
> >>>
> >>> Why?
> >>>     
> >>
> >> This directory is not mandatory. right?  
> > 
> > Clearly you've done an implementation without it, so it's not
> > functionally mandatory, but Jike has made significant code reduction
> > and simplification with it.  Those are desirable things.
> >   
> >>>>>> Lock would be still required, to handle the race conditions like
> >>>>>> 'mdev_create' is still in process and parent device is unregistered by
> >>>>>> vendor driver/ parent device is unbind from vendor driver.
> >>>>>>      
> >>>>>
> >>>>> locks are provided to protect resources, would you elaborate more on
> >>>>> what is the exact resource you want to protect by a lock in mdev_create?
> >>>>>       
> >>>>
> >>>> Simple, in your suggestion mdev-host device. Fake device will go away if
> >>>> vendor driver unregisters the device from mdev module, right.    
> >>>
> >>> I don't follow the reply here, but aiui there's ordering implicit in
> >>> the device core that Jike is trying to take advantage of that
> >>> simplifies the mdev layer significantly.  In the case of an
> >>> mdev_create, the device core needs to take a reference to the parent
> >>> object, the mdev-host I'd guess in Jike's version, the created mdev
> >>> device would also have a reference to that object, so the physical host
> >>> device could not be removed so long as there are outstanding
> >>> references.  It's just a matter of managing references and acquiring
> >>> and releasing objects.  Thanks,
> >>>    
> >>
> >> I do think this could be simplified without having extra node.  
> > 
> > The simplification is really what I'm after, whether or not it includes
> > an extra device node is not something I'm sure I have an opinion on
> > yet.  Aren't we really just talking about an extra sysfs directory
> > node?  
> 
> No, not just extra sysfs directory. I'm trying to keep parent/child
> relationship between physical device and mdev device direct without
> having extra device node.

But it's just a difference of whether the mdev devices are rooted at
the parent itself or a child of the parent.  It's just a slight
organizational change, isn't it?

> > Doesn't it make it easier for userspace to efficiently identify
> > all the mdev children when they're segregated from the other attributes
> > and sub-nodes of a parent device?
> >    
> 
> Physical devices are generally leaf nodes. I think its easier to find
> all mdev children in its own directory.

So we're just going to drop a bunch of UUID links in the parent device
sysfs directory, userspace needs to follow the link to figure out what
they are?  Doesn't that seem messy?  SR-IOV VFs are prefixed with
"virtfn" in the parent directory, putting them under an mdev-host
device is just another way to implement that organization.

> >>> the created mdev
> >>> device would also have a reference to that object, so the physical host
> >>> device could not be removed so long as there are outstanding
> >>> references.    
> >>
> >> Yes, this is also true when physical device is direct parent of mdev
> >> device. mdev device keeps reference of parent, so physical host device
> >> could not be removed as long as mdev devices are present. That is why
> >> from mdev_unregister_device() a chance is given to free all child mdev
> >> devices.  
> > 
> > But why aren't we using the device core do do that?  It seems like
> > we're getting hung up on this device node, which is more of a stylistic
> > and sysfs layout issue when the primary comment is to simplify the mdev
> > infrastructure by taking more advantage of the parent/child
> > dependencies of the device core.  Thanks,
> >   
> 
> Definitely we would use device core parent/child dependency and simplify
> mdev framework without having extra device node.

That's great, I wasn't sure if you were rejecting the whole
parent/child aspect or just the extra node.  Still, I don't see the
mdev-host node as an outright bad idea, it seems like just a way to
classify a set of child devices.  In the SR-IOV case the VFs are actual
PCI devices spawned from the parent device, so making them direct
children of the PF might make sense.  Here we're going through this
mdev core interface to create mdev devices and an mdev-host device also
seems like a realistic representation of that.  In any case, it seems
like a fairly minor tweak to add or remove it, we can argue one way or
the other on the next draft.  Thanks,

Alex