Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
Jens Axboe writes:
> On Fri, Feb 08 2013, Rusty Russell wrote:
>> Paolo Bonzini writes:
>> > 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.
>>
>> Hi Paulo,
>>
>> Note that you've defined your problem in terms of your solution
>> here. For clarity:
>>
>> The problem: we want to prepend and append to a scatterlist. We can't
>> append, because the chained scatterlist implementation requires
>> an element to be appended to join two scatterlists together.
>>
>> The solution: fix scatterlists by introducing struct sg_ring:
>> struct sg_ring {
>> struct list_head ring;
>> unsigned int nents;
>> unsigned int orig_nents; /* do we want to replace sg_table? */
>> struct scatterlist *sg;
>> };
>
> This would definitely be more flexible than the current chaining.
> However:
>
>> The workaround: make virtio accept multiple scatterlists for a single
>> buffer.
>>
>> There's nothing wrong with your workaround, but if other subsystems have
>> the same problem we do, perhaps we should consider a broader solution?
>
> Do other use cases actually exist? I don't think I've come across this
> requirement before, since it was introduced (6 years ago, from a cursory
> look at the git logs!).
Thanks Jens.
OK, let's not over-solve the problem then, we'll make a virtio-specific
solution.
Paulo, I'll take your patches once you repost.
Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
On Fri, Feb 08 2013, Rusty Russell wrote:
> Paolo Bonzini writes:
> > 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.
>
> Hi Paulo,
>
> Note that you've defined your problem in terms of your solution
> here. For clarity:
>
> The problem: we want to prepend and append to a scatterlist. We can't
> append, because the chained scatterlist implementation requires
> an element to be appended to join two scatterlists together.
>
> The solution: fix scatterlists by introducing struct sg_ring:
> struct sg_ring {
> struct list_head ring;
> unsigned int nents;
> unsigned int orig_nents; /* do we want to replace sg_table? */
> struct scatterlist *sg;
> };
This would definitely be more flexible than the current chaining.
However:
> The workaround: make virtio accept multiple scatterlists for a single
> buffer.
>
> There's nothing wrong with your workaround, but if other subsystems have
> the same problem we do, perhaps we should consider a broader solution?
Do other use cases actually exist? I don't think I've come across this
requirement before, since it was introduced (6 years ago, from a cursory
look at the git logs!).
--
Jens Axboe
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
Il 08/02/2013 05:05, Rusty Russell ha scritto:
> Paolo Bonzini writes:
>> 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.
>
> Hi Paulo,
>
> Note that you've defined your problem in terms of your solution
> here. For clarity:
Good catch. :)
> The problem: we want to prepend and append to a scatterlist. We can't
> append, because the chained scatterlist implementation requires
> an element to be appended to join two scatterlists together.
>
> The solution: fix scatterlists by introducing struct sg_ring:
> struct sg_ring {
> struct list_head ring;
> unsigned int nents;
> unsigned int orig_nents; /* do we want to replace sg_table? */
> struct scatterlist *sg;
> };
>
> The workaround: make virtio accept multiple scatterlists for a single
> buffer.
>
> There's nothing wrong with your workaround, but if other subsystems have
> the same problem we do, perhaps we should consider a broader solution?
Do they? Given the resistance you have had on the topic, perhaps they
don't (though I agree that chained scatterlist are horrible).
But I'll add a note on this to the commit message and why the workaround
is IMHO acceptable.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
Paolo Bonzini writes:
> 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.
Hi Paulo,
Note that you've defined your problem in terms of your solution
here. For clarity:
The problem: we want to prepend and append to a scatterlist. We can't
append, because the chained scatterlist implementation requires
an element to be appended to join two scatterlists together.
The solution: fix scatterlists by introducing struct sg_ring:
struct sg_ring {
struct list_head ring;
unsigned int nents;
unsigned int orig_nents; /* do we want to replace sg_table? */
struct scatterlist *sg;
};
The workaround: make virtio accept multiple scatterlists for a single
buffer.
There's nothing wrong with your workaround, but if other subsystems have
the same problem we do, perhaps we should consider a broader solution?
Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
Il 07/02/2013 14:31, Michael S. Tsirkin ha scritto: > > Single means *this piece* (for example a request header) is single. It > > could still end up in an indirect buffer because QEMU does not support > > mixed direct/indirect buffers. > > Yes but why is the optimization worth it? > It makes sense if all we want to do is add a single buffer > in one go, this would give us virtqueue_add_buf_single. > > But if we are building up an s/g list anyway, > speeding up one of the entries a tiny bit > seems very unlikely to be measureable. > No? There is some optimization potential even in unrolling the loop, but yes, it looks like I misunderstood. I'll add virtqueue_add_buf_single instead. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
On Thu, Feb 07, 2013 at 02:20:53PM +0100, Paolo Bonzini wrote: > Il 07/02/2013 14:23, Michael S. Tsirkin ha scritto: > > On Thu, Feb 07, 2013 at 02:14:24PM +0100, Paolo Bonzini wrote: > >> Il 07/02/2013 14:09, Michael S. Tsirkin ha scritto: > 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/ > >> > >> Two changes: 1) added virtqueue_add_sg_single; 2) reimplemented > >> virtqueue_add_buf in terms of the new API, which requires virtio-blk and > >> virtio-net changes. > >> > >> The virtio-blk and virtio-net changes are based on some ideas in the > >> patch Rusty posted, but virtio-net is a bit simpler and virtio-blk was > >> redone from scratch. > >> > >>> Generally we have code for direct and indirect which is already > >>> painful. We do not want 4 more variants of this code. > >> > >> Yes, indeed, the other main difference is that I'm now reimplementing > >> virtqueue_add_buf using the new functions. So: > >> > >> - we previously had 2 variants (direct/indirect) > >> > >> - v1 had 4 variants (direct/indirect x add_buf/add_sg) > >> > >> - v2 has 4 variants (direct/indirect x add_sg/add_sg_single) > > > > single is never indirect so should have a single variant. > > Single means *this piece* (for example a request header) is single. It > could still end up in an indirect buffer because QEMU does not support > mixed direct/indirect buffers. > > Paolo Yes but why is the optimization worth it? It makes sense if all we want to do is add a single buffer in one go, this would give us virtqueue_add_buf_single. But if we are building up an s/g list anyway, speeding up one of the entries a tiny bit seems very unlikely to be measureable. No? > 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. > >> > >> Yes, that's the other possibility. Will do for the next submission. > >> > >> Paolo > >> > 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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
Il 07/02/2013 14:23, Michael S. Tsirkin ha scritto: > On Thu, Feb 07, 2013 at 02:14:24PM +0100, Paolo Bonzini wrote: >> Il 07/02/2013 14:09, Michael S. Tsirkin ha scritto: 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/ >> >> Two changes: 1) added virtqueue_add_sg_single; 2) reimplemented >> virtqueue_add_buf in terms of the new API, which requires virtio-blk and >> virtio-net changes. >> >> The virtio-blk and virtio-net changes are based on some ideas in the >> patch Rusty posted, but virtio-net is a bit simpler and virtio-blk was >> redone from scratch. >> >>> Generally we have code for direct and indirect which is already >>> painful. We do not want 4 more variants of this code. >> >> Yes, indeed, the other main difference is that I'm now reimplementing >> virtqueue_add_buf using the new functions. So: >> >> - we previously had 2 variants (direct/indirect) >> >> - v1 had 4 variants (direct/indirect x add_buf/add_sg) >> >> - v2 has 4 variants (direct/indirect x add_sg/add_sg_single) > > single is never indirect so should have a single variant. Single means *this piece* (for example a request header) is single. It could still end up in an indirect buffer because QEMU does not support mixed direct/indirect buffers. Paolo 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. >> >> Yes, that's the other possibility. Will do for the next submission. >> >> Paolo >> 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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
On Thu, Feb 07, 2013 at 02:14:24PM +0100, Paolo Bonzini wrote: > Il 07/02/2013 14:09, Michael S. Tsirkin ha scritto: > >> 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/ > > Two changes: 1) added virtqueue_add_sg_single; 2) reimplemented > virtqueue_add_buf in terms of the new API, which requires virtio-blk and > virtio-net changes. > > The virtio-blk and virtio-net changes are based on some ideas in the > patch Rusty posted, but virtio-net is a bit simpler and virtio-blk was > redone from scratch. > > > Generally we have code for direct and indirect which is already > > painful. We do not want 4 more variants of this code. > > Yes, indeed, the other main difference is that I'm now reimplementing > virtqueue_add_buf using the new functions. So: > > - we previously had 2 variants (direct/indirect) > > - v1 had 4 variants (direct/indirect x add_buf/add_sg) > > - v2 has 4 variants (direct/indirect x add_sg/add_sg_single) single is never indirect so should have a single variant. > >> 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. > > Yes, that's the other possibility. Will do for the next submission. > > Paolo > > >> 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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
Il 07/02/2013 14:09, Michael S. Tsirkin ha scritto: >> 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/ Two changes: 1) added virtqueue_add_sg_single; 2) reimplemented virtqueue_add_buf in terms of the new API, which requires virtio-blk and virtio-net changes. The virtio-blk and virtio-net changes are based on some ideas in the patch Rusty posted, but virtio-net is a bit simpler and virtio-blk was redone from scratch. > Generally we have code for direct and indirect which is already > painful. We do not want 4 more variants of this code. Yes, indeed, the other main difference is that I'm now reimplementing virtqueue_add_buf using the new functions. So: - we previously had 2 variants (direct/indirect) - v1 had 4 variants (direct/indirect x add_buf/add_sg) - v2 has 4 variants (direct/indirect x add_sg/add_sg_single) >> 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. Yes, that's the other possibility. Will do for the next submission. Paolo >> 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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes
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. 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*. 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(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
