Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 22, 2012 at 10:34:13AM +0100, Paolo Bonzini wrote:
> Il 21/11/2012 19:32, Stefan Hajnoczi ha scritto:
> > The iov_get_ptr() data returns a pointer to contiguous data within a
> > vector.  This allows the caller to manipulate data inside the vector
> > without copying in/out using iov_from_buf()/iov_to_buf() when we know
> > that data is contiguous within an iovec element.
> 
> This works for you because you have a single byte to write.  It would
> not work for the SG_IO inhdr, which would need iov_to_buf().
> 
> What about the following alternative API:
> 
> void *iov_get_ptr(struct iovec *iov, unsigned int iov_cnt,
>                   ssize_t offset, size_t *bytes);
> 
> which would place the number of valid bytes (i.e. the length of the
> remainder of the iovec entry) in *bytes?
> 
> Also, I think that offset == iov_size(iov, iov_cnt) should be
> acceptable, and it would be the only case in which *bytes == 0.
> 
> Otherwise looks good.
> 
> Paolo

All this looks suspiciously like premature optimization to me.
Do we have data to show avoiding header copy is a win?

The caller would have to handle the case where the header is not
contigious, which seems just too complex - in practice the code
seems to simply fail in this case.

Why not just copy the header using iov_from_buf()/iov_to_buf()
and be done with it?


> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  iov.c | 25 +++++++++++++++++++++++++
> >  iov.h | 11 +++++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/iov.c b/iov.c
> > index 6eed089..bc78c34 100644
> > --- a/iov.c
> > +++ b/iov.c
> > @@ -395,3 +395,28 @@ size_t iov_discard(struct iovec **iov, unsigned int *iov_cnt, ssize_t bytes)
> >      }
> >      return total;
> >  }
> > +
> > +void *iov_get_ptr(struct iovec *iov, unsigned int iov_cnt,
> > +                  ssize_t offset, size_t bytes)
> > +{
> > +    if (offset < 0) {
> > +        offset += iov_size(iov, iov_cnt);
> > +        if (offset < 0) {
> > +            return NULL; /* offset before beginning of vector */
> > +        }
> > +    }
> > +
> > +    while (iov_cnt > 0) {
> > +        if (offset < iov->iov_len) {
> > +            if (bytes > iov->iov_len - offset) {
> > +                return NULL; /* would span iovec elements */
> > +            }
> > +            return iov->iov_base + offset;
> > +        }
> > +
> > +        offset -= iov->iov_len;
> > +        iov_cnt--;
> > +        iov++;
> > +    }
> > +    return NULL; /* offset beyond end of vector */
> > +}
> > diff --git a/iov.h b/iov.h
> > index d6d1fa6..674dd51 100644
> > --- a/iov.h
> > +++ b/iov.h
> > @@ -108,3 +108,14 @@ unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> >   * smaller than requested if the vector is too small.
> >   */
> >  size_t iov_discard(struct iovec **iov, unsigned int *iov_cnt, ssize_t bytes);
> > +
> > +/*
> > + * Get a pointer into a vector at offset if the given number of bytes is
> > + * contiguous and not split across iovec elements.  NULL is returned if
> > + * memory would span iovec elements or exceed the length of the vector.
> > + *
> > + * The offset is ssize_t so that an offset from the end of the vector can
> > + * be specified with a negative number.
> > + */
> > +void *iov_get_ptr(struct iovec *iov, unsigned int iov_cnt, ssize_t offset,
> > +                  size_t bytes);
> >