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

[Jike Song <jike.song@intel.com>]

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 <kwankhede@nvidia.com> wrote:
>>
>>> On 9/8/2016 1:39 PM, Jike Song wrote:
>>>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:  
>>>
>>>>>  +---------------+
>>>>>  |               |
>>>>>  | +-----------+ |  mdev_register_driver() +--------------+
>>>>>  | |           | +<------------------------+ __init()     |
>>>>>  | |  mdev     | |                         |              |
>>>>>  | |  bus      | +------------------------>+              |<-> VFIO user
>>>>>  | |  driver   | |     probe()/remove()    | vfio_mdev.ko |    APIs
>>>>>  | |           | |                         |              |
>>>>>  | +-----------+ |                         +--------------+
>>>>>  |               |  
>>>>
>>>> This aimed to have only one single vfio bus driver for all mediated devices,
>>>> right?
>>>>  
>>>
>>> Yes. That's correct.
>>>
>>>
>>>>> +
>>>>> +static int mdev_add_attribute_group(struct device *dev,
>>>>> +				    const struct attribute_group **groups)
>>>>> +{
>>>>> +	return sysfs_create_groups(&dev->kobj, groups);
>>>>> +}
>>>>> +
>>>>> +static void mdev_remove_attribute_group(struct device *dev,
>>>>> +					const struct attribute_group **groups)
>>>>> +{
>>>>> +	sysfs_remove_groups(&dev->kobj, groups);
>>>>> +}  
>>>>
>>>> These functions are not necessary. You can always specify the attribute groups
>>>> to dev->groups before registering a new device.
>>>>   
>>>
>>> At the time of mdev device create, I specifically didn't used
>>> dev->groups because we callback in vendor driver before that, see below
>>> code snippet, and those attributes should only be added if create()
>>> callback returns success.
>>>
>>>         ret = parent->ops->create(mdev, mdev_params);
>>>         if (ret)
>>>                 return ret;
>>>
>>>         ret = mdev_add_attribute_group(&mdev->dev,
>>>                                         parent->ops->mdev_attr_groups);
>>>         if (ret)
>>>                 parent->ops->destroy(mdev);
>>>
>>>
>>>
>>>>> +
>>>>> +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

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

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

> With the new changes/discussion, we believe the locking will be
> simplified without having fake parent device.
>
> With fake device suggestion, removed pointer to parent device from
> mdev_device structure. When a create(struct mdev_device *mdev) callback
> comes to vendor driver, how would vendor driver know for which physical
> device this mdev device create call is intended to? because then
> 'parent' would be newly introduced fake device, not the real parent.

Please have a look at "Figure 1".

--
Thanks,
Jike