On Wed, Feb 21, 2018 at 7:26 PM, Sowmini Varadhan
<sowmini.varad...@oracle.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.
