> From: gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx> > Sent: Tuesday, May 17, 2022 9:54 AM > > On Tue, May 17, 2022 at 12:21:03PM +0000, Parav Pandit wrote: > > Hi Greg, Yongji, > > > > > From: Yongji Xie <xieyongji@xxxxxxxxxxxxx> > > > Sent: Tuesday, May 17, 2022 3:25 AM > > > > > > On Tue, May 17, 2022 at 4:06 AM Parav Pandit <parav@xxxxxxxxxx> > wrote: > > > > > > > > > > > > > > > > > From: Xie Yongji <xieyongji@xxxxxxxxxxxxx> > > > > > Sent: Monday, May 16, 2022 2:04 AM > > > > > > > > > > Introduce a device object for vdpa management device to control > > > > > its lifecycle. > > > > Why is this needed? > > > > What is the limitation of current approach that requires new > > > > device for > > > mgmtdev? > > > > The primary motivation is missing in the commit log here. > > > > > > > > > > OK, I will add one. This patch actually comes from the discussion: > > > > > > > > > https://www.spinics.net/lists/linux-virtualization/msg56371.html > > > > > > The "vdpa_mgmt_dev" makes things a little confusing. Embedding the > > > device struct in it to control its lifecycle simplifies the logic > > > and makes the driver model clear. > > > > > No. it doesn't. > > > > vdpa_mgmt_dev is really the handle for all the netlink socket messages > targeted. > > It is not really a struct device. > > Why can it not be one? It has lifetime rules that must be followed, so might > as well use the built-in code to handle this, right? > Lifetime rules are followed. That is, during unloading of the driver for the struct device* dev, mgmtdev must be unregistered. Following the exact mirror of creation. I general before deleting the struct device *dev, its upper layer user mgmt. dev must be unregistered. There will be zero user of this struct mgmt_dev, after invocation of current API vdpa_mgmtdev_unregister(). This is the onus on above unregister API. Any cleanup cannot be differed until newly thought dev->release(). > What is wrong with it being a struct device? I am not sure if its fully wrong. First for this struct device, there is no real driver bind to it. Secondly, it is simply an overkill without any visible use. vdpa_mgmt_dev struct has only register/unregister life cycle. Unregistration of it stops and destroy any outstanding operation on it. Any code accessing mgmtdev post unregistration simply won't work. Anything pending cannot be freed in the newly sighted release() callback. Freeing anything in the release is too late because hw resources are destroy after unregister and before release(). Contrary to it, doing so will hide bugs because such struct will live longer now if done as struct device. On a side note, these devices are in 500 to 1K range and expected to grow more. Creating yet another struct device and linking it to its parent would result in additional sysfs links, udev event explosion for no absolute use or doesn't bring any object lifecycle changes. vduse is a special case in the kernel, that has to create a dummy device because there is no real device (like pci) linked to it that can service vdpa commands. > > > We can rename it to vdpa_mgmt_handle, if the _dev suffix is confusing. > > Where is the "management" device if this is not that? > > What does "handle" mean here? It a handle to on which commands are arriving via socket from user space. These commands are serviced by the struct device *dev. DMA operation etc also happen on *dev. You can think of it as unique handle to refer to the kernel like netdev->ifindex on netlink messages. > > > And regarding vduse_dev_release() and existing empty release function, > they can be dynamically allocated. > > This is because they are really the struct device. > > I do not understand this statement, sorry. It was bad sentence, my bad. What I wanted to say is, probably better explained with real patch below. In context of [1], struct vduse_mgmtdev empty release function can be avoided by below inline patch [2]. [1] https://www.spinics.net/lists/linux-virtualization/msg56371.html [2] patch: >From f87d557fe939a1632473dd11526a87301adbab8d Mon Sep 17 00:00:00 2001 From: Parav Pandit <parav@xxxxxxxxxx> Date: Wed, 18 May 2022 01:22:21 +0300 Subject: [PATCH] vduse: Tie vduse mgmtdev and its device vduse management device is not backed by any real device such as PCI. Hence it doesn't have any parent device linked to it. Kernel driver model in [1] suggests avoiding an empty device release callback. Hence tie the mgmtdevice object's life cycle to an allocate dummy struct device instead of having it static. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/kobject.rst?h=v5.18-rc7#n284 Signed-off-by: Parav Pandit <parav@xxxxxxxxxx> --- drivers/vdpa/vdpa_user/vduse_dev.c | 60 ++++++++++++++++++------------ 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c index f85d1a08ed87..ebe272575fb8 100644 --- a/drivers/vdpa/vdpa_user/vduse_dev.c +++ b/drivers/vdpa/vdpa_user/vduse_dev.c @@ -1475,16 +1475,12 @@ static char *vduse_devnode(struct device *dev, umode_t *mode) return kasprintf(GFP_KERNEL, "vduse/%s", dev_name(dev)); } -static void vduse_mgmtdev_release(struct device *dev) -{ -} - -static struct device vduse_mgmtdev = { - .init_name = "vduse", - .release = vduse_mgmtdev_release, +struct vduse_mgmt_dev { + struct vdpa_mgmt_dev mgmt_dev; + struct device dev; }; -static struct vdpa_mgmt_dev mgmt_dev; +static struct vduse_mgmt_dev *vduse_mgmt; static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name) { @@ -1509,7 +1505,7 @@ static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name) } set_dma_ops(&vdev->vdpa.dev, &vduse_dev_dma_ops); vdev->vdpa.dma_dev = &vdev->vdpa.dev; - vdev->vdpa.mdev = &mgmt_dev; + vdev->vdpa.mdev = &vduse_mgmt->mgmt_dev; return 0; } @@ -1555,34 +1551,52 @@ static struct virtio_device_id id_table[] = { { 0 }, }; -static struct vdpa_mgmt_dev mgmt_dev = { - .device = &vduse_mgmtdev, - .id_table = id_table, - .ops = &vdpa_dev_mgmtdev_ops, -}; +static void vduse_mgmtdev_release(struct device *dev) +{ + struct vduse_mgmt_dev *mgmt_dev; + + mgmt_dev = container_of(dev, struct vduse_mgmt_dev, dev); + kfree(mgmt_dev); +} static int vduse_mgmtdev_init(void) { int ret; - ret = device_register(&vduse_mgmtdev); - if (ret) + vduse_mgmt = kzalloc(sizeof(*vduse_mgmt), GFP_KERNEL); + if (!vduse_mgmt) + return -ENOMEM; + + ret = dev_set_name(&vduse_mgmt->dev, "vduse-la"); + if (ret) { + kfree(vduse_mgmt); return ret; + } - ret = vdpa_mgmtdev_register(&mgmt_dev); + vduse_mgmt->dev.release = vduse_mgmtdev_release; + + ret = device_register(&vduse_mgmt->dev); if (ret) - goto err; + goto dev_reg_err; - return 0; -err: - device_unregister(&vduse_mgmtdev); + vduse_mgmt->mgmt_dev.id_table = id_table; + vduse_mgmt->mgmt_dev.ops = &vdpa_dev_mgmtdev_ops; + vduse_mgmt->mgmt_dev.device = &vduse_mgmt->dev; + ret = vdpa_mgmtdev_register(&vduse_mgmt->mgmt_dev); + if (ret) + device_unregister(&vduse_mgmt->dev); + + return ret; + +dev_reg_err: + put_device(&vduse_mgmt->dev); return ret; } static void vduse_mgmtdev_exit(void) { - vdpa_mgmtdev_unregister(&mgmt_dev); - device_unregister(&vduse_mgmtdev); + vdpa_mgmtdev_unregister(&vduse_mgmt->mgmt_dev); + device_unregister(&vduse_mgmt->dev); } static int vduse_init(void) -- 2.26.2 _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
