From: "Michael S. Tsirkin" <mst@redhat.com> To: Paolo Bonzini <pbonzini@redhat.com> Cc: linux-kernel@vger.kernel.org, Wanlong Gao <gaowanlong@cn.fujitsu.com>, asias@redhat.com, Rusty Russell <rusty@rustcorp.com.au>, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes Date: Thu, 7 Feb 2013 15:09:12 +0200 [thread overview] Message-ID: <20130207130912.GA20875@redhat.com> (raw) In-Reply-To: <1360239752-2470-1-git-send-email-pbonzini@redhat.com> On Thu, Feb 07, 2013 at 01:22:24PM +0100, Paolo Bonzini wrote: > The virtqueue_add_buf function has two limitations: > > 1) it requires the caller to provide all the buffers in a single call; > > 2) it does not support chained scatterlists: the buffers must be > provided as an array of struct scatterlist. > > Because of these limitations, virtio-scsi has to copy each request into > a scatterlist internal to the driver. It cannot just use the one that > was prepared by the upper SCSI layers. > > This series adds a different set of APIs for adding a buffer to a > virtqueue. The new API lets you pass the buffers piecewise, wrapping > multiple calls to virtqueue_add_sg between virtqueue_start_buf and > virtqueue_end_buf. Letting drivers call virtqueue_add_sg multiple times > if they already have a scatterlist provided by someone else simplifies the > code and, for virtio-scsi, it saves the copying and the related locking. > > One major difference between virtqueue_add_buf and virtqueue_add_sg > is that the latter uses scatterlist iterators, which follow chained > scatterlist structs and stop at ending markers. In order to avoid code > duplication, and use the new API from virtqueue_add_buf (patch 8), we need > to change all existing callers of virtqueue_add_buf to provide well-formed > scatterlists. This is what patches 2-7 do. For virtio-blk it is easiest > to just switch to the new API, just like for virtio-scsi. For virtio-net > the ending marker must be reset after calling virtqueue_add_buf, in > preparation for the next usage of the scatterlist. Other drivers are > safe already. > What are the changes as compared to the previous version? How about some comments made on the previous version? See e.g. https://patchwork.kernel.org/patch/1891541/ Generally we have code for direct and indirect which is already painful. We do not want 4 more variants of this code. > This is an RFC for two reasons. First, because I haven't done enough > testing yet (especially with all the variations on receiving that > virtio-net has). Second, because I still have two struct vring_desc * > fields in virtqueue API, which is a layering violation. I'm not really > sure how important that is and how to fix that---except by making the > fields void*. Hide the whole structure as part of vring struct, the problem will go away. > Paolo > Paolo Bonzini (8): > virtio: add functions for piecewise addition of buffers > virtio-blk: reorganize virtblk_add_req > virtio-blk: use virtqueue_start_buf on bio path > virtio-blk: use virtqueue_start_buf on req path > scatterlist: introduce sg_unmark_end > virtio-net: unmark scatterlist ending after virtqueue_add_buf > virtio-scsi: use virtqueue_start_buf > virtio: reimplement virtqueue_add_buf using new functions > > block/blk-integrity.c | 2 +- > block/blk-merge.c | 2 +- > drivers/block/virtio_blk.c | 165 +++++++++-------- > drivers/net/virtio_net.c | 21 ++- > drivers/scsi/virtio_scsi.c | 103 +++++------ > drivers/virtio/virtio_ring.c | 417 +++++++++++++++++++++++++++--------------- > include/linux/scatterlist.h | 16 ++ > include/linux/virtio.h | 25 +++ > 8 files changed, 460 insertions(+), 291 deletions(-)
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com> To: Paolo Bonzini <pbonzini@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes Date: Thu, 7 Feb 2013 15:09:12 +0200 [thread overview] Message-ID: <20130207130912.GA20875@redhat.com> (raw) In-Reply-To: <1360239752-2470-1-git-send-email-pbonzini@redhat.com> On Thu, Feb 07, 2013 at 01:22:24PM +0100, Paolo Bonzini wrote: > The virtqueue_add_buf function has two limitations: > > 1) it requires the caller to provide all the buffers in a single call; > > 2) it does not support chained scatterlists: the buffers must be > provided as an array of struct scatterlist. > > Because of these limitations, virtio-scsi has to copy each request into > a scatterlist internal to the driver. It cannot just use the one that > was prepared by the upper SCSI layers. > > This series adds a different set of APIs for adding a buffer to a > virtqueue. The new API lets you pass the buffers piecewise, wrapping > multiple calls to virtqueue_add_sg between virtqueue_start_buf and > virtqueue_end_buf. Letting drivers call virtqueue_add_sg multiple times > if they already have a scatterlist provided by someone else simplifies the > code and, for virtio-scsi, it saves the copying and the related locking. > > One major difference between virtqueue_add_buf and virtqueue_add_sg > is that the latter uses scatterlist iterators, which follow chained > scatterlist structs and stop at ending markers. In order to avoid code > duplication, and use the new API from virtqueue_add_buf (patch 8), we need > to change all existing callers of virtqueue_add_buf to provide well-formed > scatterlists. This is what patches 2-7 do. For virtio-blk it is easiest > to just switch to the new API, just like for virtio-scsi. For virtio-net > the ending marker must be reset after calling virtqueue_add_buf, in > preparation for the next usage of the scatterlist. Other drivers are > safe already. > What are the changes as compared to the previous version? How about some comments made on the previous version? See e.g. https://patchwork.kernel.org/patch/1891541/ Generally we have code for direct and indirect which is already painful. We do not want 4 more variants of this code. > This is an RFC for two reasons. First, because I haven't done enough > testing yet (especially with all the variations on receiving that > virtio-net has). Second, because I still have two struct vring_desc * > fields in virtqueue API, which is a layering violation. I'm not really > sure how important that is and how to fix that---except by making the > fields void*. Hide the whole structure as part of vring struct, the problem will go away. > Paolo > Paolo Bonzini (8): > virtio: add functions for piecewise addition of buffers > virtio-blk: reorganize virtblk_add_req > virtio-blk: use virtqueue_start_buf on bio path > virtio-blk: use virtqueue_start_buf on req path > scatterlist: introduce sg_unmark_end > virtio-net: unmark scatterlist ending after virtqueue_add_buf > virtio-scsi: use virtqueue_start_buf > virtio: reimplement virtqueue_add_buf using new functions > > block/blk-integrity.c | 2 +- > block/blk-merge.c | 2 +- > drivers/block/virtio_blk.c | 165 +++++++++-------- > drivers/net/virtio_net.c | 21 ++- > drivers/scsi/virtio_scsi.c | 103 +++++------ > drivers/virtio/virtio_ring.c | 417 +++++++++++++++++++++++++++--------------- > include/linux/scatterlist.h | 16 ++ > include/linux/virtio.h | 25 +++ > 8 files changed, 460 insertions(+), 291 deletions(-)
next prev parent reply other threads:[~2013-02-07 13:05 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-02-07 12:22 [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:22 ` [RFC PATCH 1/8] virtio: add functions for piecewise addition of buffers Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:22 ` [RFC PATCH 2/8] virtio-blk: reorganize virtblk_add_req Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:22 ` [RFC PATCH 3/8] virtio-blk: use virtqueue_start_buf on bio path Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:22 ` [RFC PATCH 4/8] virtio-blk: use virtqueue_start_buf on req path Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:22 ` [RFC PATCH 5/8] scatterlist: introduce sg_unmark_end Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:35 ` Jens Axboe 2013-02-07 12:35 ` Jens Axboe 2013-02-07 12:22 ` [RFC PATCH 6/8] virtio-net: unmark scatterlist ending after virtqueue_add_buf Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:22 ` [RFC PATCH 7/8] virtio-scsi: use virtqueue_start_buf Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 12:22 ` [RFC PATCH 8/8] virtio: reimplement virtqueue_add_buf using new functions Paolo Bonzini 2013-02-07 12:22 ` Paolo Bonzini 2013-02-07 13:09 ` Michael S. Tsirkin [this message] 2013-02-07 13:09 ` [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes Michael S. Tsirkin 2013-02-07 13:14 ` Paolo Bonzini 2013-02-07 13:14 ` Paolo Bonzini 2013-02-07 13:23 ` Michael S. Tsirkin 2013-02-07 13:23 ` Michael S. Tsirkin 2013-02-07 13:20 ` Paolo Bonzini 2013-02-07 13:20 ` Paolo Bonzini 2013-02-07 13:31 ` Michael S. Tsirkin 2013-02-07 13:31 ` Michael S. Tsirkin 2013-02-07 13:30 ` Paolo Bonzini 2013-02-07 13:30 ` Paolo Bonzini 2013-02-08 4:05 ` Rusty Russell 2013-02-08 4:05 ` Rusty Russell 2013-02-08 6:35 ` Paolo Bonzini 2013-02-08 6:35 ` Paolo Bonzini 2013-02-08 11:52 ` Jens Axboe 2013-02-08 11:52 ` Jens Axboe 2013-02-13 9:46 ` Rusty Russell 2013-02-13 9:46 ` Rusty Russell
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=20130207130912.GA20875@redhat.com \ --to=mst@redhat.com \ --cc=asias@redhat.com \ --cc=gaowanlong@cn.fujitsu.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=pbonzini@redhat.com \ --cc=rusty@rustcorp.com.au \ --cc=virtualization@lists.linux-foundation.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.