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

[Kirti Wankhede]

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.

>>>>>> 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.

> 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.

>>> 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.

Thanks,
Kirti