RE: [PATCH V5 4/9] Drivers: hv: ring_buffer: enhance hv_ringbuffer_read() to support hvsock

[Dexuan Cui <decui@microsoft.com>]

> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, January 5, 2016 20:31
> ...
> > To get the payload of hvsock, we need raw=0 to skip the level-1 header
> > (i.e., struct vmpacket_descriptor desc) and we also need to skip the
> > level-2 header (i.e., struct vmpipe_proto_header pipe_hdr).
> >
> > NB: if the length of the hvsock payload is not aligned with the 8-byte
> > boundeary, at most 7 padding bytes are appended, so the real hvsock
> > payload's length must be retrieved by the pipe_hdr.data_size field.
> >
> > I 'upgrade' the 'raw' parameter of hv_ringbuffer_read() to a
> > 'read_flags', trying to share the logic of the function.
> 
> When I was touching this code last time I was actually thinking about
> eliminating 'raw' flag by making all ring reads raw and moving this
> header filtering job to the upper layer (as we already have
> vmbus_recvpacket()/vmbus_recvpacket_raw()) but for some reason I didn't
> do it. I believe you have more or less the same reasoing for introducing
> new read type instead of parsing this at a higher level. Some comments
> below ...

I feel it's more convenient to do the parsing in the vmbus driver than in
all the driver users of vmbus driver.

However, yes, I admit hv_ringbuffer_read() becomes less readable with
my introduction of 'read_flags'.

It may be a better idea to do the parsing in higher level, i.e., the hvsock driver,
in my case.
It looks I can avoid introducing vmbus_recvpacket_hvsock() and use 
vmbus_recvpacket() directly in my hvsock driver.

Let me try to make a new patch this way.

> > This patch is required by the next patch, which will introduce the hvsock
> > send/recv APIs.
> >
> > ...
> > @@ -619,9 +619,20 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info
> *ring_info,
> >  		    struct kvec *kv_list,
> >  		    u32 kv_count, bool *signal);
> >
> > +/*
> > + * By default, a read_flags of 0 means: the payload offset is
> > + * sizeof(struct vmpacket_descriptor).
> > + *
> > + * If HV_RINGBUFFER_READ_FLAG_RAW is used, the payload offset is 0.
> > + *
> > + * If HV_RINGBUFFER_READ_FLAG_HVSOCK is used, the payload offset is
> > + * sizeof(struct vmpacket_descriptor) + sizeof(struct
> > vmpipe_proto_header).
> 
> So these are mutually exclusive, right? Should we introduce 'int
> payload_offset' parameter instead of flags?
Sorry for making the code less readable. :-)
As I mentioned above, let me try to do things in a better way.

> > @@ -415,17 +426,26 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info
> *inring_info,
> >  		goto out_unlock;
> >  	}
> >
> > +	if (tot_hdrlen > buflen) {
> > +		ret = -ENOBUFS;
> > +		goto out_unlock;
> > +	}
> > +
> > +	desc = (struct vmpacket_descriptor *)buffer;
> > +
> >  	next_read_location = hv_get_next_read_location(inring_info);
> > -	next_read_location = hv_copyfrom_ringbuffer(inring_info, &desc,
> > -						    sizeof(desc),
> > +	next_read_location = hv_copyfrom_ringbuffer(inring_info, desc,
> > +						    tot_hdrlen,
> >  						    next_read_location);
> > +	offset = 0;
> > +	if (!raw)
> > +		offset += (desc->offset8 << 3);
> > +	if (hvsock)
> > +		offset += sizeof(*pipe_hdr);
> 
> So in case of !raw and hvsock we add both offsets?
Yes...
 
Thanks for you review, Vitaly.

Thanks,
-- Dexuan