Re: [RFC PATCH 0/8] virtio: new API for addition of buffers, scatterlist changes

["Michael S. Tsirkin" <mst@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(-)