[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0684d7e3211c464cbfe303e80dcab4b4@HKXPR3004MB0088.064d.mgd.msft.net>
Date: Tue, 5 Jan 2016 15:41:20 +0000
From: Dexuan Cui <decui@...rosoft.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"stephen@...workplumber.org" <stephen@...workplumber.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"driverdev-devel@...uxdriverproject.org"
<driverdev-devel@...uxdriverproject.org>,
"olaf@...fle.de" <olaf@...fle.de>,
"apw@...onical.com" <apw@...onical.com>,
"jasowang@...hat.com" <jasowang@...hat.com>,
KY Srinivasan <kys@...rosoft.com>,
"pebolle@...cali.nl" <pebolle@...cali.nl>,
"stefanha@...hat.com" <stefanha@...hat.com>,
"dan.carpenter@...cle.com" <dan.carpenter@...cle.com>
Subject: RE: [PATCH V5 4/9] Drivers: hv: ring_buffer: enhance
hv_ringbuffer_read() to support hvsock
> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists