netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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. 

  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).