From: Dexuan Cui <decui@microsoft.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"davem@davemloft.net" <davem@davemloft.net>,
"stephen@networkplumber.org" <stephen@networkplumber.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"driverdev-devel@linuxdriverproject.org"
<driverdev-devel@linuxdriverproject.org>,
"olaf@aepfle.de" <olaf@aepfle.de>,
"apw@canonical.com" <apw@canonical.com>,
"jasowang@redhat.com" <jasowang@redhat.com>,
KY Srinivasan <kys@microsoft.com>,
"pebolle@tiscali.nl" <pebolle@tiscali.nl>,
"stefanha@redhat.com" <stefanha@redhat.com>,
"dan.carpenter@oracle.com" <dan.carpenter@oracle.com>
Subject: RE: [PATCH V5 4/9] Drivers: hv: ring_buffer: enhance hv_ringbuffer_read() to support hvsock
Date: Tue, 5 Jan 2016 15:41:20 +0000 [thread overview]
Message-ID: <0684d7e3211c464cbfe303e80dcab4b4@HKXPR3004MB0088.064d.mgd.msft.net> (raw)
In-Reply-To: <87r3hw1a3n.fsf@vitty.brq.redhat.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
prev parent reply other threads:[~2016-01-05 15:41 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-24 14:14 [PATCH V5 4/9] Drivers: hv: ring_buffer: enhance hv_ringbuffer_read() to support hvsock Dexuan Cui
2016-01-02 4:29 ` David Miller
2016-01-03 11:55 ` Dexuan Cui
2016-01-05 12:31 ` Vitaly Kuznetsov
2016-01-05 15:41 ` Dexuan Cui [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0684d7e3211c464cbfe303e80dcab4b4@HKXPR3004MB0088.064d.mgd.msft.net \
--to=decui@microsoft.com \
--cc=apw@canonical.com \
--cc=dan.carpenter@oracle.com \
--cc=davem@davemloft.net \
--cc=driverdev-devel@linuxdriverproject.org \
--cc=gregkh@linuxfoundation.org \
--cc=jasowang@redhat.com \
--cc=kys@microsoft.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olaf@aepfle.de \
--cc=pebolle@tiscali.nl \
--cc=stefanha@redhat.com \
--cc=stephen@networkplumber.org \
--cc=vkuznets@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).