From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Tian, Kevin" Subject: RE: [RFC PATCH v4 1/3] Mediated device Core driver Date: Fri, 27 May 2016 09:00:17 +0000 Message-ID: References: <1464119897-10844-1-git-send-email-kwankhede@nvidia.com> <1464119897-10844-2-git-send-email-kwankhede@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Cc: "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" , "Ruan, Shuai" , "Song, Jike" , "Lv, Zhiyuan" , "bjsdjshi@linux.vnet.ibm.com" To: Kirti Wankhede , "alex.williamson@redhat.com" , "pbonzini@redhat.com" , "kraxel@redhat.com" , "cjia@nvidia.com" Return-path: Received: from mga14.intel.com ([192.55.52.115]:34599 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932235AbcE0JAZ convert rfc822-to-8bit (ORCPT ); Fri, 27 May 2016 05:00:25 -0400 In-Reply-To: Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: > From: Kirti Wankhede > Sent: Wednesday, May 25, 2016 10:47 PM > > > >> +static struct devices_list { > >> + struct list_head dev_list; > >> + struct mutex list_lock; > >> +} mdevices, phy_devices; > > > > phy_devices -> pdevices? and similarly we can use pdev/mdev > > pair in other places... > > > > 'pdevices' sometimes also refers to 'pointer to devices' that's the > reason I perfer to use phy_devices to represent 'physical devices' well, I think it should be clear in this context where 'p' means 'physical'. Just like frequently used pdev in pci files for pci_dev... > > > >> +static struct mdev_device *find_mdev_device(uuid_le uuid, int instance) > > > > can we just call it "struct mdev* or "mdevice"? "dev_device" looks > redundant. > > > > 'struct mdev_device' represents 'device structure for device created by > mdev module'. Still that doesn't satisfy major folks, I'm open to change > it. IMO 'mdev' should be clearly enough to represent a mediated device > > > > Sorry I may have to ask same question since I didn't get an answer yet. > > what exactly does 'instance' mean here? since uuid is unique, why do > > we need match instance too? > > > > 'uuid' could be UUID of a VM for whom it is created. To support mutiple > mediated devices for same VM, name should be unique. Hence we need a > instance number to identify each mediated device uniquely in one VM. If UUID alone cannot universally identify a mediated device, what's the point of explicitly tagging it in kernel? Either we assign a new UUID for this mdev itself, or possibly better define it as a string? Management stack can pass any ID/name in string format which is sufficiently to identify mdev to its own purpose? Then in this framework we just do simple string match... > > > > >> + if (phy_dev->ops->destroy) { > >> + if (phy_dev->ops->destroy(phy_dev->dev, mdevice->uuid, > >> + mdevice->instance)) { > >> + mutex_unlock(&phy_devices.list_lock); > > > > a warning message is preferred. Also better to return -EBUSY here. > > > > mdev_destroy_device() is called from 2 paths, one is sysfs mdev_destroy > and mdev_unregister_device(). For the later case, return from here will > any ways ignored. mdev_unregister_device() is called from the remove > function of physical device and that doesn't care about return error, it > just removes the device from subsystem. Regardless of whether caller will handle error, this function itself should return error since it makes sense in other path e.g. sysfs to let user know what's happening. > > >> + return; > >> + } > >> + } > >> + > >> + mdev_remove_attribute_group(&mdevice->dev, > >> + phy_dev->ops->mdev_attr_groups); > >> + mdevice->phy_dev = NULL; > > > > Am I missing something here? You didn't remove this mdev node from > > the list, and below... > > > > device_unregister() calls put_device(dev) and if refcount is zero its > release function is called, which is mdev_device_release(), that is > hooked during device_register(). This node is removed from list from > mdev_device_release(). I'm not sure whether there'll be some race condition here, since you put a defunc mdev on the list... > > >> + phy_dev->dev = dev; > >> + phy_dev->ops = ops; > >> + > >> + mutex_lock(&phy_devices.list_lock); > >> + ret = mdev_create_sysfs_files(dev); > >> + if (ret) > >> + goto add_sysfs_error; > >> + > >> + ret = mdev_add_attribute_group(dev, ops->dev_attr_groups); > >> + if (ret) > >> + goto add_group_error; > > > > any reason to include sysfs operations inside the mutex which is > > purely about phy_devices list? > > > > dev_attr_groups attribute is for physical device, hence inside > phy_devices.list_lock. phy_devices.list_lock only protects the list, when you plan to add a new phy_device node after it's initialized and get ready. sysfs attribute setup is still part of initialization. Thanks Kevin From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43671) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b6DdD-0002ld-KS for qemu-devel@nongnu.org; Fri, 27 May 2016 05:00:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b6Dd6-0005IS-A5 for qemu-devel@nongnu.org; Fri, 27 May 2016 05:00:34 -0400 Received: from mga04.intel.com ([192.55.52.120]:60739) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b6Dd5-0005Hi-Vm for qemu-devel@nongnu.org; Fri, 27 May 2016 05:00:28 -0400 From: "Tian, Kevin" Date: Fri, 27 May 2016 09:00:17 +0000 Message-ID: References: <1464119897-10844-1-git-send-email-kwankhede@nvidia.com> <1464119897-10844-2-git-send-email-kwankhede@nvidia.com> In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [RFC PATCH v4 1/3] Mediated device Core driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirti Wankhede , "alex.williamson@redhat.com" , "pbonzini@redhat.com" , "kraxel@redhat.com" , "cjia@nvidia.com" Cc: "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" , "Ruan, Shuai" , "Song, Jike" , "Lv, Zhiyuan" , "bjsdjshi@linux.vnet.ibm.com" > From: Kirti Wankhede > Sent: Wednesday, May 25, 2016 10:47 PM >=20 >=20 > >> +static struct devices_list { > >> + struct list_head dev_list; > >> + struct mutex list_lock; > >> +} mdevices, phy_devices; > > > > phy_devices -> pdevices? and similarly we can use pdev/mdev > > pair in other places... > > >=20 > 'pdevices' sometimes also refers to 'pointer to devices' that's the > reason I perfer to use phy_devices to represent 'physical devices' well, I think it should be clear in this context where 'p' means 'physical'= . Just like frequently used pdev in pci files for pci_dev... >=20 >=20 > >> +static struct mdev_device *find_mdev_device(uuid_le uuid, int instanc= e) > > > > can we just call it "struct mdev* or "mdevice"? "dev_device" looks > redundant. > > >=20 > 'struct mdev_device' represents 'device structure for device created by > mdev module'. Still that doesn't satisfy major folks, I'm open to change > it. IMO 'mdev' should be clearly enough to represent a mediated device >=20 >=20 > > Sorry I may have to ask same question since I didn't get an answer yet. > > what exactly does 'instance' mean here? since uuid is unique, why do > > we need match instance too? > > >=20 > 'uuid' could be UUID of a VM for whom it is created. To support mutiple > mediated devices for same VM, name should be unique. Hence we need a > instance number to identify each mediated device uniquely in one VM. If UUID alone cannot universally identify a mediated device, what's the point of explicitly tagging it in kernel? Either we assign a new UUID for this mdev itself, or possibly better define it as a string? Management=20 stack can pass any ID/name in string format which is sufficiently to identi= fy=20 mdev to its own purpose? Then in this framework we just do simple string match... >=20 >=20 >=20 > >> + if (phy_dev->ops->destroy) { > >> + if (phy_dev->ops->destroy(phy_dev->dev, mdevice->uuid, > >> + mdevice->instance)) { > >> + mutex_unlock(&phy_devices.list_lock); > > > > a warning message is preferred. Also better to return -EBUSY here. > > >=20 > mdev_destroy_device() is called from 2 paths, one is sysfs mdev_destroy > and mdev_unregister_device(). For the later case, return from here will > any ways ignored. mdev_unregister_device() is called from the remove > function of physical device and that doesn't care about return error, it > just removes the device from subsystem. Regardless of whether caller will handle error, this function itself should return error since it makes sense in other path e.g. sysfs to let user know what's happening. >=20 > >> + return; > >> + } > >> + } > >> + > >> + mdev_remove_attribute_group(&mdevice->dev, > >> + phy_dev->ops->mdev_attr_groups); > >> + mdevice->phy_dev =3D NULL; > > > > Am I missing something here? You didn't remove this mdev node from > > the list, and below... > > >=20 > device_unregister() calls put_device(dev) and if refcount is zero its > release function is called, which is mdev_device_release(), that is > hooked during device_register(). This node is removed from list from > mdev_device_release(). I'm not sure whether there'll be some race condition here, since you put a defunc mdev on the list...=20 >=20 > >> + phy_dev->dev =3D dev; > >> + phy_dev->ops =3D ops; > >> + > >> + mutex_lock(&phy_devices.list_lock); > >> + ret =3D mdev_create_sysfs_files(dev); > >> + if (ret) > >> + goto add_sysfs_error; > >> + > >> + ret =3D mdev_add_attribute_group(dev, ops->dev_attr_groups); > >> + if (ret) > >> + goto add_group_error; > > > > any reason to include sysfs operations inside the mutex which is > > purely about phy_devices list? > > >=20 > dev_attr_groups attribute is for physical device, hence inside > phy_devices.list_lock. phy_devices.list_lock only protects the list, when you plan to add a new phy_device node after it's initialized and get ready. sysfs attribute setup is still part of initialization. Thanks Kevin