From: "Jakub Staroń" <jstaron@google.com> To: Pankaj Gupta <pagupta@redhat.com>, linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-acpi@vger.kernel.org, qemu-devel@nongnu.org, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org Cc: jack@suse.cz, mst@redhat.com, jasowang@redhat.com, david@fromorbit.com, lcapitulino@redhat.com, adilger.kernel@dilger.ca, zwisler@kernel.org, aarcange@redhat.com, dave.jiang@intel.com, darrick.wong@oracle.com, vishal.l.verma@intel.com, david@redhat.com, willy@infradead.org, hch@infradead.org, jmoyer@redhat.com, nilal@redhat.com, lenb@kernel.org, kilobyte@angband.pl, riel@surriel.com, yuval.shaia@oracle.com, stefanha@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, kwolf@redhat.com, tytso@mit.edu, xiaoguangrong.eric@gmail.com, cohuck@redhat.com, rjw@rjwysocki.net, imammedo@redhat.com, smbarber@google.com Subject: Re: [Qemu-devel] [PATCH v7 2/6] virtio-pmem: Add virtio pmem driver Date: Tue, 7 May 2019 13:25:54 -0700 [thread overview] Message-ID: <3d6479ae-6c39-d614-f1d9-aa1978e2e438@google.com> (raw) In-Reply-To: <20190426050039.17460-3-pagupta@redhat.com> On 4/25/19 10:00 PM, Pankaj Gupta wrote: > +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); Shouldn't it be rather `list_del(vpmem->req_list.next)`? We are trying to unlink first element of the list and `vpmem->req_list` is just the list head. > +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(); > + 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); > + 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); > + } Aren't the arguments in `list_add_tail` swapped? The element we are adding should be first, the list should be second. Also, shouldn't we resubmit the request after waking up from `wait_event(req->wq_buf, req->wq_buf_avail)`? I propose rewriting it like that: diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c index 66b582f751a3..ff0556b04e86 100644 --- a/drivers/nvdimm/virtio_pmem.c +++ b/drivers/nvdimm/virtio_pmem.c @@ -25,7 +25,7 @@ void host_ack(struct virtqueue *vq) if (!list_empty(&vpmem->req_list)) { req_buf = list_first_entry(&vpmem->req_list, struct virtio_pmem_request, list); - list_del(&vpmem->req_list); + list_del(vpmem->req_list.next); req_buf->wq_buf_avail = true; wake_up(&req_buf->wq_buf); } @@ -59,17 +59,33 @@ int virtio_pmem_flush(struct nd_region *nd_region) sgs[1] = &ret; spin_lock_irqsave(&vpmem->pmem_lock, flags); - err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC); - if (err) { - dev_err(&vdev->dev, "failed to send command to virtio pmem device\n"); + /* + * If virtqueue_add_sgs returns -ENOSPC then req_vq virtual queue does not + * have free descriptor slots. We add the request to req_list and wait + * for host_ack to wake us up when free slots are available. + */ + while ((err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC)) == -ENOSPC) { + dev_err(&vdev->dev, "failed to send command to virtio pmem device, no free slots in the virtqueue, postponing request\n"); + req->wq_buf_avail = false; - list_add_tail(&vpmem->req_list, &req->list); + list_add_tail(&req->list, &vpmem->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_add_sgs failed with error different than -ENOSPC, we can't + * do anything about that. + */ + if (err) { + dev_info(&vdev->dev, "failed to send command to virtio pmem device, error code %d\n", err); + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); + err = -EIO; + goto ret; + } err = virtqueue_kick(vpmem->req_vq); spin_unlock_irqrestore(&vpmem->pmem_lock, flags); Let me know if it looks reasonable to you. Thank you, Jakub Staron
WARNING: multiple messages have this Message-ID (diff)
From: "Jakub Staroń via Qemu-devel" <qemu-devel@nongnu.org> To: Pankaj Gupta <pagupta@redhat.com>, linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-acpi@vger.kernel.org, qemu-devel@nongnu.org, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org Cc: jack@suse.cz, mst@redhat.com, jasowang@redhat.com, david@fromorbit.com, lcapitulino@redhat.com, adilger.kernel@dilger.ca, smbarber@google.com, zwisler@kernel.org, aarcange@redhat.com, dave.jiang@intel.com, darrick.wong@oracle.com, vishal.l.verma@intel.com, david@redhat.com, willy@infradead.org, hch@infradead.org, jmoyer@redhat.com, nilal@redhat.com, lenb@kernel.org, kilobyte@angband.pl, riel@surriel.com, yuval.shaia@oracle.com, stefanha@redhat.com, imammedo@redhat.com, dan.j.williams@intel.com, kwolf@redhat.com, tytso@mit.edu, xiaoguangrong.eric@gmail.com, cohuck@redhat.com, rjw@rjwysocki.net, pbonzini@redhat.com Subject: Re: [Qemu-devel] [PATCH v7 2/6] virtio-pmem: Add virtio pmem driver Date: Tue, 7 May 2019 13:25:54 -0700 [thread overview] Message-ID: <3d6479ae-6c39-d614-f1d9-aa1978e2e438@google.com> (raw) In-Reply-To: <20190426050039.17460-3-pagupta@redhat.com> On 4/25/19 10:00 PM, Pankaj Gupta wrote: > +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); Shouldn't it be rather `list_del(vpmem->req_list.next)`? We are trying to unlink first element of the list and `vpmem->req_list` is just the list head. > +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(); > + 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); > + 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); > + } Aren't the arguments in `list_add_tail` swapped? The element we are adding should be first, the list should be second. Also, shouldn't we resubmit the request after waking up from `wait_event(req->wq_buf, req->wq_buf_avail)`? I propose rewriting it like that: diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c index 66b582f751a3..ff0556b04e86 100644 --- a/drivers/nvdimm/virtio_pmem.c +++ b/drivers/nvdimm/virtio_pmem.c @@ -25,7 +25,7 @@ void host_ack(struct virtqueue *vq) if (!list_empty(&vpmem->req_list)) { req_buf = list_first_entry(&vpmem->req_list, struct virtio_pmem_request, list); - list_del(&vpmem->req_list); + list_del(vpmem->req_list.next); req_buf->wq_buf_avail = true; wake_up(&req_buf->wq_buf); } @@ -59,17 +59,33 @@ int virtio_pmem_flush(struct nd_region *nd_region) sgs[1] = &ret; spin_lock_irqsave(&vpmem->pmem_lock, flags); - err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC); - if (err) { - dev_err(&vdev->dev, "failed to send command to virtio pmem device\n"); + /* + * If virtqueue_add_sgs returns -ENOSPC then req_vq virtual queue does not + * have free descriptor slots. We add the request to req_list and wait + * for host_ack to wake us up when free slots are available. + */ + while ((err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC)) == -ENOSPC) { + dev_err(&vdev->dev, "failed to send command to virtio pmem device, no free slots in the virtqueue, postponing request\n"); + req->wq_buf_avail = false; - list_add_tail(&vpmem->req_list, &req->list); + list_add_tail(&req->list, &vpmem->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_add_sgs failed with error different than -ENOSPC, we can't + * do anything about that. + */ + if (err) { + dev_info(&vdev->dev, "failed to send command to virtio pmem device, error code %d\n", err); + spin_unlock_irqrestore(&vpmem->pmem_lock, flags); + err = -EIO; + goto ret; + } err = virtqueue_kick(vpmem->req_vq); spin_unlock_irqrestore(&vpmem->pmem_lock, flags); Let me know if it looks reasonable to you. Thank you, Jakub Staron
next prev parent reply other threads:[~2019-05-07 20:25 UTC|newest] Thread overview: 107+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-26 5:00 [PATCH v7 0/6] virtio pmem driver Pankaj Gupta 2019-04-26 5:00 ` [Qemu-devel] " Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` [PATCH v7 1/6] libnvdimm: nd_region flush callback support Pankaj Gupta 2019-04-26 5:00 ` [Qemu-devel] " Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` [PATCH v7 2/6] virtio-pmem: Add virtio pmem driver Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` [Qemu-devel] " Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta [not found] ` <20190426050039.17460-3-pagupta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2019-04-30 5:53 ` [Qemu-devel] " Yuval Shaia 2019-04-30 5:53 ` Yuval Shaia 2019-04-30 5:53 ` Yuval Shaia 2019-04-30 6:06 ` Pankaj Gupta 2019-04-30 6:06 ` Pankaj Gupta 2019-04-30 6:06 ` Pankaj Gupta 2019-04-30 6:06 ` Pankaj Gupta 2019-04-30 5:53 ` Yuval Shaia 2019-05-07 15:35 ` Dan Williams 2019-05-07 15:35 ` Dan Williams 2019-05-07 15:35 ` [Qemu-devel] " Dan Williams 2019-05-07 15:35 ` Dan Williams 2019-05-07 15:35 ` Dan Williams [not found] ` <CAPcyv4hdT5bbgv0Gy1r0Xb3RMfE_Zpe7DV10a=F1PFeTeEt+Fw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2019-05-08 11:19 ` [Qemu-devel] " Pankaj Gupta 2019-05-08 11:19 ` Pankaj Gupta 2019-05-08 11:19 ` Pankaj Gupta 2019-05-10 23:33 ` Dan Williams 2019-05-10 23:33 ` Dan Williams 2019-05-10 23:33 ` Dan Williams 2019-05-10 23:33 ` Dan Williams 2019-05-11 1:26 ` Pankaj Gupta 2019-05-11 1:26 ` Pankaj Gupta 2019-05-11 1:26 ` Pankaj Gupta 2019-05-11 1:26 ` Pankaj Gupta 2019-05-08 11:19 ` Pankaj Gupta 2019-05-07 20:25 ` Jakub Staroń [this message] 2019-05-07 20:25 ` Jakub Staroń via Qemu-devel 2019-05-08 11:12 ` Pankaj Gupta 2019-05-08 11:12 ` Pankaj Gupta 2019-05-08 11:12 ` Pankaj Gupta 2019-05-08 11:12 ` Pankaj Gupta 2019-05-08 15:23 ` Pankaj Gupta 2019-05-08 15:23 ` Pankaj Gupta 2019-05-08 15:23 ` Pankaj Gupta 2019-05-08 15:23 ` Pankaj Gupta 2019-05-08 19:05 ` Jakub Staroń 2019-05-08 19:05 ` Jakub Staroń via Qemu-devel 2019-05-08 19:05 ` Jakub Staroń 2019-05-08 19:05 ` Jakub Staroń via Virtualization 2019-05-07 20:25 ` Jakub Staroń via Virtualization 2019-04-26 5:00 ` [PATCH v7 3/6] libnvdimm: add dax_dev sync flag Pankaj Gupta 2019-04-26 5:00 ` [Qemu-devel] " Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-05-07 15:40 ` Dan Williams 2019-05-07 15:40 ` [Qemu-devel] " Dan Williams 2019-05-07 15:40 ` Dan Williams 2019-05-07 15:40 ` Dan Williams 2019-05-09 12:24 ` Pankaj Gupta 2019-05-09 12:24 ` [Qemu-devel] " Pankaj Gupta 2019-05-09 12:24 ` Pankaj Gupta 2019-05-09 12:24 ` Pankaj Gupta 2019-05-07 15:40 ` Dan Williams 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` [PATCH v7 4/6] dax: check synchronous mapping is supported Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` [Qemu-devel] " Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-05-07 19:24 ` [Qemu-devel] " Jakub Staroń via Virtualization 2019-05-07 19:24 ` Jakub Staroń 2019-05-07 19:24 ` Jakub Staroń via Qemu-devel 2019-05-07 19:24 ` Jakub Staroń 2019-05-08 5:31 ` Pankaj Gupta 2019-05-08 5:31 ` Pankaj Gupta 2019-05-08 5:31 ` Pankaj Gupta 2019-05-08 5:31 ` Pankaj Gupta 2019-05-08 5:31 ` Pankaj Gupta 2019-04-26 5:00 ` [PATCH v7 5/6] ext4: disable map_sync for async flush Pankaj Gupta 2019-04-26 5:00 ` [Qemu-devel] " Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` [PATCH v7 6/6] xfs: " Pankaj Gupta 2019-04-26 5:00 ` [Qemu-devel] " Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta 2019-05-07 15:37 ` Dan Williams 2019-05-07 15:37 ` Dan Williams 2019-05-07 15:37 ` [Qemu-devel] " Dan Williams 2019-05-07 15:37 ` Dan Williams 2019-05-07 15:37 ` Dan Williams 2019-05-07 16:17 ` Darrick J. Wong 2019-05-07 16:17 ` [Qemu-devel] " Darrick J. Wong 2019-05-07 16:17 ` Darrick J. Wong 2019-05-07 16:17 ` Darrick J. Wong 2019-05-08 5:49 ` [Qemu-devel] " Pankaj Gupta 2019-05-08 5:49 ` Pankaj Gupta 2019-05-08 5:49 ` Pankaj Gupta 2019-05-08 5:49 ` Pankaj Gupta 2019-05-08 5:49 ` Pankaj Gupta 2019-04-26 5:00 ` Pankaj Gupta
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3d6479ae-6c39-d614-f1d9-aa1978e2e438@google.com \ --to=jstaron@google.com \ --cc=aarcange@redhat.com \ --cc=adilger.kernel@dilger.ca \ --cc=cohuck@redhat.com \ --cc=dan.j.williams@intel.com \ --cc=darrick.wong@oracle.com \ --cc=dave.jiang@intel.com \ --cc=david@fromorbit.com \ --cc=david@redhat.com \ --cc=hch@infradead.org \ --cc=imammedo@redhat.com \ --cc=jack@suse.cz \ --cc=jasowang@redhat.com \ --cc=jmoyer@redhat.com \ --cc=kilobyte@angband.pl \ --cc=kvm@vger.kernel.org \ --cc=kwolf@redhat.com \ --cc=lcapitulino@redhat.com \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvdimm@lists.01.org \ --cc=linux-xfs@vger.kernel.org \ --cc=mst@redhat.com \ --cc=nilal@redhat.com \ --cc=pagupta@redhat.com \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=riel@surriel.com \ --cc=rjw@rjwysocki.net \ --cc=smbarber@google.com \ --cc=stefanha@redhat.com \ --cc=tytso@mit.edu \ --cc=virtualization@lists.linux-foundation.org \ --cc=vishal.l.verma@intel.com \ --cc=willy@infradead.org \ --cc=xiaoguangrong.eric@gmail.com \ --cc=yuval.shaia@oracle.com \ --cc=zwisler@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.