On 9/19/2016 11:41 PM, Alex Williamson wrote:
> On Mon, 19 Sep 2016 22:59:34 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>
>> On 9/12/2016 9:23 PM, Alex Williamson wrote:
>>> On Mon, 12 Sep 2016 13:19:11 +0530
>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> 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 <kwankhede@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
