From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from userp2120.oracle.com ([156.151.31.85]:53016 "EHLO
        userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751650AbeBYQUs (ORCPT
        <rfc822;netdev@vger.kernel.org>); Sun, 25 Feb 2018 11:20:48 -0500
Date: Sun, 25 Feb 2018 11:20:27 -0500
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>,
        Santosh Shilimkar <santosh.shilimkar@oracle.com>
Subject: Re: [PATCH V2 net-next 2/3] rds: deliver zerocopy completion
 notification with data
Message-ID: <20180225162027.GA32483@oracle.com>
References: <cover.1519399473.git.sowmini.varadhan@oracle.com>
 <e3be990c88dbac833852ab9b430035a568acd929.1519399473.git.sowmini.varadhan@oracle.com>
 <CAF=yD-JEYZ=nw0eFNTHJW3zZVCzYRtJ0yGL90_Mgjay4HcVLXQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAF=yD-JEYZ=nw0eFNTHJW3zZVCzYRtJ0yGL90_Mgjay4HcVLXQ@mail.gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On (02/25/18 10:56), Willem de Bruijn wrote:
> > @@ -91,22 +85,19 @@ static void rds_rm_zerocopy_callback(struct rds_sock *rs,
> >                 spin_unlock_irqrestore(&q->lock, flags);
> >                 mm_unaccount_pinned_pages(&znotif->z_mmp);
> >                 consume_skb(rds_skb_from_znotifier(znotif));
> > -               sk->sk_error_report(sk);
> > +               /* caller should wake up POLLIN */
> 
> sk->sk_data_ready(sk);

yes, this was my first thought, but everything else in rds
is calling rds_wake_sk_sleep (this is even done in
rds_recv_incoming(), which actually queues up the data), 
so I chose to align with that model (and call this in the caller 
of rds_rm_zerocopy_callback()

> Without the error queue, the struct no longer needs to be an skb,
> per se. Converting to a different struct with list_head is definitely
> a longer patch. But kmalloc will be cheaper than alloc_skb.
> Perhaps something to try (as separate follow-on work).

right, I was thinking along these exact lines as well,
and was already planning a follow-up.

> > +       if (!sock_flag(rds_rs_to_sk(rs), SOCK_ZEROCOPY) || !skb_peek(q))
> > +               return 0;
> 
> Racy read?

Can you elaborate? I only put the skb_peek to quickly
bail for sockets that are not using zerocopy at all- 
if you race against something that's queuing data, and 
miss it on the peek, the next read/recv should find it.
Am I missing some race?


> 
> > +
> > +       if (!msg->msg_control ||
> 
> I'd move this first, so that the cookie queue need not even be probed
> in the common case.

you mean before the check for SOCK_ZEROCOPY?

> > +           msg->msg_controllen < CMSG_SPACE(sizeof(*done)))
> > +               return 0;
> 
> if caller does not satisfy the contract on controllen size, can be
> more explicit and return an error.

if SOCK_ZEROCOPY has been set, but the recv did not specify a cmsghdr,
you mean?

> > +               ncookies = rds_recvmsg_zcookie(rs, msg);

Will take care of the remaining comments in V3.