On Sun, Feb 25, 2018 at 11:20 AM, Sowmini Varadhan <sowmini.varad...@oracle.com> wrote: > 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()
Ah, understood. Perhaps say "wakes" instead of "should wake". I mistakenly read this as a todo. >> 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? It''s a lockless access. But intentionally so, then. You're right, as long as the subsequent skb_dequeue handles the case where the queue is empty, it seems okay to optimistically probe lockless first. >> >> > + >> > + 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? Yes >> > + 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? I mean if SOCK_ZEROCOPY has been set and the caller calls recvmsg with a control buffer, but one that is too small to handle zerocopy cookie notifications. >> > + ncookies = rds_recvmsg_zcookie(rs, msg); > > Will take care of the remaining comments in V3.
