netdev - Re: [PATCH net-next] RDS: deliver zerocopy completion notification with data as an optimization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-+ZRyGmQ1z2ss-HWWy=LrZfmzjbG+KkRH1gbNSVVun-MA@mail.gmail.com>
Date:   Wed, 21 Feb 2018 19:39:13 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, rds-devel@....oracle.com,
        Santosh Shilimkar <santosh.shilimkar@...cle.com>
Subject: Re: [PATCH net-next] RDS: deliver zerocopy completion notification
 with data as an optimization

On Wed, Feb 21, 2018 at 7:26 PM, Sowmini Varadhan
<sowmini.varadhan@...cle.com> wrote:
> On (02/21/18 18:45), Willem de Bruijn wrote:
>>
>> I do mean returning 0 instead of -EAGAIN if control data is ready.
>> Something like
>>
>> @@ -611,7 +611,8 @@ int rds_recvmsg(struct socket *sock, struct msghdr
>> *msg, size_t size,
>>
>>                 if (!rds_next_incoming(rs, &inc)) {
>>                         if (nonblock) {
>> -                               ret = -EAGAIN;
>> +                               ncookies = rds_recvmsg_zcookie(rs, msg);
>> +                               ret = ncookies ? 0 : -EAGAIN;
>>                                 break;
>>                         }
>
> Yes, but you now have an implicit branch based on ncookies, so I'm
> not sure it saved all that much?

At least it removes the extra list empty check in the hot path and
relegates this to the relatively rare branch when the queue is empty
and the syscall is non-blocking.

> like I said let me revisit this

Okay. I won't harp on this further.

>> By the way, the put_cmsg is unconditional even if the caller did
>> not supply msg_control. So it is basically no longer safe to ever
>> call read, recv or recvfrom on a socket if zerocopy notifications
>> are outstanding.
>
> Wait, I thought put_cmsg already checks for these things.

It does, and sets MSG_CTRUNC to signal that it was unable to
write all control data. But by then the notifications have already
been dequeued.

>> It is possible to check msg_controllen before even deciding whether
>> to try to dequeue notifications (and take the lock). I see that this is
>> not common. But RDS of all cases seems to do this, in
>> rds_notify_queue_get:
>
> yes the comment above that code suggests that this bit of code
> was done to avoid calling put_cmsg while holding the rs_lock.
>
> One bit of administrivia though- if I now drop sk_error_queue for
> PF_RDS, I'll have to fix selftests in the same patch too, so the
> patch will get a bit bulky (and thus a bit more difficult to review).

Understood. It might be cleanest to split into three patches. One
revert of the error queue code, one new feature and one update
to the test.