Re: [Qemu-devel] [PATCH v4 2/5] virtio-pmem: Add virtio pmem driver
On Wed, Apr 03, 2019 at 08:40:13AM -0400, Pankaj Gupta wrote: > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] virtio-pmem: Add virtio pmem driver > > > > On Wed, Apr 03, 2019 at 04:10:15PM +0530, Pankaj Gupta wrote: > > > This patch adds virtio-pmem driver for KVM guest. > > > > > > Guest reads the persistent memory range information from > > > Qemu over VIRTIO and registers it on nvdimm_bus. It also > > > creates a nd_region object with the persistent memory > > > range information so that existing 'nvdimm/pmem' driver > > > can reserve this into system memory map. This way > > > 'virtio-pmem' driver uses existing functionality of pmem > > > driver to register persistent memory compatible for DAX > > > capable filesystems. > > > > > > This also provides function to perform guest flush over > > > VIRTIO from 'pmem' driver when userspace performs flush > > > on DAX memory range. > > > > > > Signed-off-by: Pankaj Gupta > > > --- > > > drivers/nvdimm/virtio_pmem.c | 84 + > > > drivers/virtio/Kconfig | 10 +++ > > > drivers/virtio/Makefile | 1 + > > > drivers/virtio/pmem.c| 125 +++ > > > include/linux/virtio_pmem.h | 60 +++ > > > include/uapi/linux/virtio_ids.h | 1 + > > > include/uapi/linux/virtio_pmem.h | 10 +++ > > > 7 files changed, 291 insertions(+) > > > create mode 100644 drivers/nvdimm/virtio_pmem.c > > > create mode 100644 drivers/virtio/pmem.c > > > create mode 100644 include/linux/virtio_pmem.h > > > create mode 100644 include/uapi/linux/virtio_pmem.h > > > > > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > > > new file mode 100644 > > > index ..2a1b1ba2c1ff > > > --- /dev/null > > > +++ b/drivers/nvdimm/virtio_pmem.c > > > @@ -0,0 +1,84 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > > Is this comment stile (//) acceptable? > > In existing code, i can see same comment > pattern for license at some places. Is it preferred for new code? > > > > > > +/* > > > + * virtio_pmem.c: Virtio pmem Driver > > > + * > > > + * Discovers persistent memory range information > > > + * from host and provides a virtio based flushing > > > + * interface. > > > + */ > > > +#include > > > +#include "nd.h" > > > + > > > + /* The interrupt handler */ > > > +void host_ack(struct virtqueue *vq) > > > +{ > > > + unsigned int len; > > > + unsigned long flags; > > > + struct virtio_pmem_request *req, *req_buf; > > > + struct virtio_pmem *vpmem = vq->vdev->priv; > > > + > > > + spin_lock_irqsave(&vpmem->pmem_lock, flags); > > > + while ((req = virtqueue_get_buf(vq, &len)) != NULL) { > > > + req->done = true; > > > + wake_up(&req->host_acked); > > > + > > > + if (!list_empty(&vpmem->req_list)) { > > > + req_buf = list_first_entry(&vpmem->req_list, > > > + struct virtio_pmem_request, list); > > > + list_del(&vpmem->req_list); > > > + req_buf->wq_buf_avail = true; > > > + wake_up(&req_buf->wq_buf); > > > + } > > > + } > > > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); > > > +} > > > +EXPORT_SYMBOL_GPL(host_ack); > > > + > > > + /* The request submission function */ > > > +int virtio_pmem_flush(struct nd_region *nd_region) > > > +{ > > > + int err; > > > + unsigned long flags; > > > + struct scatterlist *sgs[2], sg, ret; > > > + struct virtio_device *vdev = nd_region->provider_data; > > > + struct virtio_pmem *vpmem = vdev->priv; > > > + struct virtio_pmem_request *req; > > > + > > > + might_sleep(); > > > > [1] > > > > > + req = kmalloc(sizeof(*req), GFP_KERNEL); > > > + if (!req) > > > + return -ENOMEM; > > > + > > > + req->done = req->wq_buf_avail = false; > > > + strcpy(req->name, "FLUSH"); > > > + init_waitqueue_head(&req->host_acked); > > > + init_waitqueue_head(&req->wq_buf); > &
Re: [Qemu-devel] [PATCH v4 2/5] virtio-pmem: Add virtio pmem driver
On Wed, Apr 03, 2019 at 04:10:15PM +0530, Pankaj Gupta wrote: > This patch adds virtio-pmem driver for KVM guest. > > Guest reads the persistent memory range information from > Qemu over VIRTIO and registers it on nvdimm_bus. It also > creates a nd_region object with the persistent memory > range information so that existing 'nvdimm/pmem' driver > can reserve this into system memory map. This way > 'virtio-pmem' driver uses existing functionality of pmem > driver to register persistent memory compatible for DAX > capable filesystems. > > This also provides function to perform guest flush over > VIRTIO from 'pmem' driver when userspace performs flush > on DAX memory range. > > Signed-off-by: Pankaj Gupta > --- > drivers/nvdimm/virtio_pmem.c | 84 + > drivers/virtio/Kconfig | 10 +++ > drivers/virtio/Makefile | 1 + > drivers/virtio/pmem.c| 125 +++ > include/linux/virtio_pmem.h | 60 +++ > include/uapi/linux/virtio_ids.h | 1 + > include/uapi/linux/virtio_pmem.h | 10 +++ > 7 files changed, 291 insertions(+) > create mode 100644 drivers/nvdimm/virtio_pmem.c > create mode 100644 drivers/virtio/pmem.c > create mode 100644 include/linux/virtio_pmem.h > create mode 100644 include/uapi/linux/virtio_pmem.h > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > new file mode 100644 > index ..2a1b1ba2c1ff > --- /dev/null > +++ b/drivers/nvdimm/virtio_pmem.c > @@ -0,0 +1,84 @@ > +// SPDX-License-Identifier: GPL-2.0 Is this comment stile (//) acceptable? > +/* > + * virtio_pmem.c: Virtio pmem Driver > + * > + * Discovers persistent memory range information > + * from host and provides a virtio based flushing > + * interface. > + */ > +#include > +#include "nd.h" > + > + /* The interrupt handler */ > +void host_ack(struct virtqueue *vq) > +{ > + unsigned int len; > + unsigned long flags; > + struct virtio_pmem_request *req, *req_buf; > + struct virtio_pmem *vpmem = vq->vdev->priv; > + > + spin_lock_irqsave(&vpmem->pmem_lock, flags); > + while ((req = virtqueue_get_buf(vq, &len)) != NULL) { > + req->done = true; > + wake_up(&req->host_acked); > + > + if (!list_empty(&vpmem->req_list)) { > + req_buf = list_first_entry(&vpmem->req_list, > + struct virtio_pmem_request, list); > + list_del(&vpmem->req_list); > + req_buf->wq_buf_avail = true; > + wake_up(&req_buf->wq_buf); > + } > + } > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); > +} > +EXPORT_SYMBOL_GPL(host_ack); > + > + /* The request submission function */ > +int virtio_pmem_flush(struct nd_region *nd_region) > +{ > + int err; > + unsigned long flags; > + struct scatterlist *sgs[2], sg, ret; > + struct virtio_device *vdev = nd_region->provider_data; > + struct virtio_pmem *vpmem = vdev->priv; > + struct virtio_pmem_request *req; > + > + might_sleep(); [1] > + req = kmalloc(sizeof(*req), GFP_KERNEL); > + if (!req) > + return -ENOMEM; > + > + req->done = req->wq_buf_avail = false; > + strcpy(req->name, "FLUSH"); > + init_waitqueue_head(&req->host_acked); > + init_waitqueue_head(&req->wq_buf); > + sg_init_one(&sg, req->name, strlen(req->name)); > + sgs[0] = &sg; > + sg_init_one(&ret, &req->ret, sizeof(req->ret)); > + sgs[1] = &ret; > + > + spin_lock_irqsave(&vpmem->pmem_lock, flags); > + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC); Is it okay to use GFP_ATOMIC in a might-sleep ([1]) function? > + if (err) { > + dev_err(&vdev->dev, "failed to send command to virtio pmem > device\n"); > + > + list_add_tail(&vpmem->req_list, &req->list); > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); > + > + /* When host has read buffer, this completes via host_ack */ > + wait_event(req->wq_buf, req->wq_buf_avail); > + spin_lock_irqsave(&vpmem->pmem_lock, flags); > + } > + virtqueue_kick(vpmem->req_vq); You probably want to check return value here. > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); > + > + /* When host has read buffer, this completes via host_ack */ > + wait_event(req->host_acked, req->done); > + err = req->ret; > + kfree(req); > + > + return err; > +}; > +EXPORT_SYMBOL_GPL(virtio_pmem_flush); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig > index 35897649c24f..9f634a2ed638 100644 > --- a/drivers/virtio/Kconfig > +++ b/drivers/virtio/Kconfig > @@ -42,6 +42,16 @@ config VIRTIO_PCI_LEGACY > > If unsure, say Y. > > +config VIRTIO_PMEM > + tristate "Support for virtio pmem driver" > + depends on VIRTIO > + depends on LIBNVDIMM
Re: [Qemu-devel] [PATCH v4 2/5] virtio-pmem: Add virtio pmem driver
> > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] virtio-pmem: Add virtio pmem > > > driver > > > > > > On Wed, Apr 03, 2019 at 04:10:15PM +0530, Pankaj Gupta wrote: > > > > This patch adds virtio-pmem driver for KVM guest. > > > > > > > > Guest reads the persistent memory range information from > > > > Qemu over VIRTIO and registers it on nvdimm_bus. It also > > > > creates a nd_region object with the persistent memory > > > > range information so that existing 'nvdimm/pmem' driver > > > > can reserve this into system memory map. This way > > > > 'virtio-pmem' driver uses existing functionality of pmem > > > > driver to register persistent memory compatible for DAX > > > > capable filesystems. > > > > > > > > This also provides function to perform guest flush over > > > > VIRTIO from 'pmem' driver when userspace performs flush > > > > on DAX memory range. > > > > > > > > Signed-off-by: Pankaj Gupta > > > > --- > > > > drivers/nvdimm/virtio_pmem.c | 84 + > > > > drivers/virtio/Kconfig | 10 +++ > > > > drivers/virtio/Makefile | 1 + > > > > drivers/virtio/pmem.c| 125 +++ > > > > include/linux/virtio_pmem.h | 60 +++ > > > > include/uapi/linux/virtio_ids.h | 1 + > > > > include/uapi/linux/virtio_pmem.h | 10 +++ > > > > 7 files changed, 291 insertions(+) > > > > create mode 100644 drivers/nvdimm/virtio_pmem.c > > > > create mode 100644 drivers/virtio/pmem.c > > > > create mode 100644 include/linux/virtio_pmem.h > > > > create mode 100644 include/uapi/linux/virtio_pmem.h > > > > > > > > diff --git a/drivers/nvdimm/virtio_pmem.c > > > > b/drivers/nvdimm/virtio_pmem.c > > > > new file mode 100644 > > > > index ..2a1b1ba2c1ff > > > > --- /dev/null > > > > +++ b/drivers/nvdimm/virtio_pmem.c > > > > @@ -0,0 +1,84 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > Is this comment stile (//) acceptable? > > > > In existing code, i can see same comment > > pattern for license at some places. > > Is it preferred for new code? will change. > > > > > > > > > > +/* > > > > + * virtio_pmem.c: Virtio pmem Driver > > > > + * > > > > + * Discovers persistent memory range information > > > > + * from host and provides a virtio based flushing > > > > + * interface. > > > > + */ > > > > +#include > > > > +#include "nd.h" > > > > + > > > > + /* The interrupt handler */ > > > > +void host_ack(struct virtqueue *vq) > > > > +{ > > > > + unsigned int len; > > > > + unsigned long flags; > > > > + struct virtio_pmem_request *req, *req_buf; > > > > + struct virtio_pmem *vpmem = vq->vdev->priv; > > > > + > > > > + spin_lock_irqsave(&vpmem->pmem_lock, flags); > > > > + while ((req = virtqueue_get_buf(vq, &len)) != NULL) { > > > > + req->done = true; > > > > + wake_up(&req->host_acked); > > > > + > > > > + if (!list_empty(&vpmem->req_list)) { > > > > + req_buf = list_first_entry(&vpmem->req_list, > > > > + struct virtio_pmem_request, > > > > list); > > > > + list_del(&vpmem->req_list); > > > > + req_buf->wq_buf_avail = true; > > > > + wake_up(&req_buf->wq_buf); > > > > + } > > > > + } > > > > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); > > > > +} > > > > +EXPORT_SYMBOL_GPL(host_ack); > > > > + > > > > + /* The request submission function */ > > > > +int virtio_pmem_flush(struct nd_region *nd_region) > > > > +{ > > > > + int err; > > > > + unsigned long flags; > > > > + struct scatterlist *sgs[2], sg, ret; > > > > + struct virtio_device
Re: [Qemu-devel] [PATCH v4 2/5] virtio-pmem: Add virtio pmem driver
> Subject: Re: [Qemu-devel] [PATCH v4 2/5] virtio-pmem: Add virtio pmem driver > > On Wed, Apr 03, 2019 at 04:10:15PM +0530, Pankaj Gupta wrote: > > This patch adds virtio-pmem driver for KVM guest. > > > > Guest reads the persistent memory range information from > > Qemu over VIRTIO and registers it on nvdimm_bus. It also > > creates a nd_region object with the persistent memory > > range information so that existing 'nvdimm/pmem' driver > > can reserve this into system memory map. This way > > 'virtio-pmem' driver uses existing functionality of pmem > > driver to register persistent memory compatible for DAX > > capable filesystems. > > > > This also provides function to perform guest flush over > > VIRTIO from 'pmem' driver when userspace performs flush > > on DAX memory range. > > > > Signed-off-by: Pankaj Gupta > > --- > > drivers/nvdimm/virtio_pmem.c | 84 + > > drivers/virtio/Kconfig | 10 +++ > > drivers/virtio/Makefile | 1 + > > drivers/virtio/pmem.c| 125 +++ > > include/linux/virtio_pmem.h | 60 +++ > > include/uapi/linux/virtio_ids.h | 1 + > > include/uapi/linux/virtio_pmem.h | 10 +++ > > 7 files changed, 291 insertions(+) > > create mode 100644 drivers/nvdimm/virtio_pmem.c > > create mode 100644 drivers/virtio/pmem.c > > create mode 100644 include/linux/virtio_pmem.h > > create mode 100644 include/uapi/linux/virtio_pmem.h > > > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > > new file mode 100644 > > index ..2a1b1ba2c1ff > > --- /dev/null > > +++ b/drivers/nvdimm/virtio_pmem.c > > @@ -0,0 +1,84 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > Is this comment stile (//) acceptable? In existing code, i can see same comment pattern for license at some places. > > > +/* > > + * virtio_pmem.c: Virtio pmem Driver > > + * > > + * Discovers persistent memory range information > > + * from host and provides a virtio based flushing > > + * interface. > > + */ > > +#include > > +#include "nd.h" > > + > > + /* The interrupt handler */ > > +void host_ack(struct virtqueue *vq) > > +{ > > + unsigned int len; > > + unsigned long flags; > > + struct virtio_pmem_request *req, *req_buf; > > + struct virtio_pmem *vpmem = vq->vdev->priv; > > + > > + spin_lock_irqsave(&vpmem->pmem_lock, flags); > > + while ((req = virtqueue_get_buf(vq, &len)) != NULL) { > > + req->done = true; > > + wake_up(&req->host_acked); > > + > > + if (!list_empty(&vpmem->req_list)) { > > + req_buf = list_first_entry(&vpmem->req_list, > > + struct virtio_pmem_request, list); > > + list_del(&vpmem->req_list); > > + req_buf->wq_buf_avail = true; > > + wake_up(&req_buf->wq_buf); > > + } > > + } > > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); > > +} > > +EXPORT_SYMBOL_GPL(host_ack); > > + > > + /* The request submission function */ > > +int virtio_pmem_flush(struct nd_region *nd_region) > > +{ > > + int err; > > + unsigned long flags; > > + struct scatterlist *sgs[2], sg, ret; > > + struct virtio_device *vdev = nd_region->provider_data; > > + struct virtio_pmem *vpmem = vdev->priv; > > + struct virtio_pmem_request *req; > > + > > + might_sleep(); > > [1] > > > + req = kmalloc(sizeof(*req), GFP_KERNEL); > > + if (!req) > > + return -ENOMEM; > > + > > + req->done = req->wq_buf_avail = false; > > + strcpy(req->name, "FLUSH"); > > + init_waitqueue_head(&req->host_acked); > > + init_waitqueue_head(&req->wq_buf); > > + sg_init_one(&sg, req->name, strlen(req->name)); > > + sgs[0] = &sg; > > + sg_init_one(&ret, &req->ret, sizeof(req->ret)); > > + sgs[1] = &ret; > > + > > + spin_lock_irqsave(&vpmem->pmem_lock, flags); > > + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC); > > Is it okay to use GFP_ATOMIC in a might-sleep ([1]) function? might sleep will give us a warning if we try to sleep from non-sleepable context. We are doing it other way, i.e might_sl