From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Network Development <netdev@vger.kernel.org>,
David Miller <davem@davemloft.net>,
rds-devel@oss.oracle.com, santosh.shilimkar@oracle.com
Subject: Re: [PATCH net-next 7/7] selftests/net: add zerocopy support for PF_RDS test case
Date: Sun, 28 Jan 2018 11:18:51 -0500 [thread overview]
Message-ID: <20180128161851.GC22885@oracle.com> (raw)
In-Reply-To: <CAF=yD-Kg+twYbbsBnO-jKdJ8J2M_o-vfqR5NXL8WvxeU7PL0dQ@mail.gmail.com>
Re-ordering review comments for selftests a bit..
> > + cm->cmsg_level = SOL_RDS;
> > + cm->cmsg_type = RDS_CMSG_ZCOPY_COOKIE;
> > + ++cookie;
> > + memcpy(CMSG_DATA(cm), &cookie, sizeof(cookie));
> cookie is not initialized
it's a static uint32_t in the function. It will get initialized to 0.
But the whole test program is rather simplistic, since it doesnt
actually verify the value of the cookies (hopefully me pending
updates to rds-stress will provide better testing/examples in this
space, because I actully have something multi-threaded, that must
necesarily use a truly random value for the cookie, and must really
make sure that the returned value from the kernel matches some
cookie that is pending)
> > +static void add_zcopy_cookie(struct msghdr *msg)
> Please just allocate ahead of time. And since cookie size is fixed
> and small just define a local variable on the stack in do_sendmsg.
Ok! I was just trying to make this as future-proof as possible, and
provide useful example code for the next cut/paste developer to use.
> > + cm->cmsg_len = CMSG_SPACE(sizeof(cookie));
> CMSG_LEN
:
> Instead of indenting all this existing code please add a helper
> do_recv_completion_zcookie, call that if SO_EE_ORIGIN_ZCOOKIE
> and fall through to existing code otherwise.
:
> Verify ncookies <= MAX_..
> Verify ret == ncookies * sizeof(uint32_t)
> > + zerocopied = 1;
> Unused in this path
Ok, will fix all these.
Also, coalescing some related comments for patch 6/7:
> + memset(&msg, 0, sizeof(msg));
> + msg.msg_name = &din;
> + msg.msg_namelen = sizeof(din);
Not read, so no need to configure. In that case a simpler
recv will do and is more concise than setting up recvmsg.
Real RDS applications actually have to use recvmsg, since they
can get cmsg info about other things like congestion notification
so let's leave this as recvmsg - it does no harm, and provides
test coverage for the recvmsg case as well.
next prev parent reply other threads:[~2018-01-28 16:23 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-24 11:45 [PATCH net-next 0/7] RDS zerocopy support Sowmini Varadhan
2018-01-24 11:45 ` [PATCH net-next 1/7] skbuff: export mm_[un]account_pinned_pages for other modules Sowmini Varadhan
2018-01-24 11:45 ` [PATCH net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock Sowmini Varadhan
2018-01-25 14:44 ` Willem de Bruijn
2018-01-25 15:35 ` Sowmini Varadhan
2018-01-28 13:51 ` Willem de Bruijn
2018-01-28 16:18 ` Sowmini Varadhan
2018-01-28 18:54 ` Willem de Bruijn
2018-01-24 11:45 ` [PATCH net-next 3/7] sock: permit SO_ZEROCOPY on PF_RDS socket Sowmini Varadhan
2018-01-24 11:45 ` [PATCH net-next 4/7] rds: support for zcopy completion notification Sowmini Varadhan
2018-01-28 13:56 ` Willem de Bruijn
2018-01-28 16:15 ` Sowmini Varadhan
2018-01-28 18:46 ` Willem de Bruijn
2018-01-24 11:46 ` [PATCH net-next 5/7] rds: zerocopy Tx support Sowmini Varadhan
2018-01-28 13:57 ` Willem de Bruijn
2018-01-24 11:46 ` [PATCH net-next 6/7] selftests/net: add support for PF_RDS sockets Sowmini Varadhan
2018-01-28 13:58 ` Willem de Bruijn
2018-01-24 11:46 ` [PATCH net-next 7/7] selftests/net: add zerocopy support for PF_RDS test case Sowmini Varadhan
2018-01-28 14:00 ` Willem de Bruijn
2018-01-28 16:18 ` Sowmini Varadhan [this message]
2018-01-28 18:39 ` Willem de Bruijn
2018-01-28 19:57 ` Sowmini Varadhan
2018-01-25 16:41 ` [PATCH net-next 0/7] RDS zerocopy support Santosh Shilimkar
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=20180128161851.GC22885@oracle.com \
--to=sowmini.varadhan@oracle.com \
--cc=davem@davemloft.net \
--cc=netdev@vger.kernel.org \
--cc=rds-devel@oss.oracle.com \
--cc=santosh.shilimkar@oracle.com \
--cc=willemdebruijn.kernel@gmail.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).