| Message ID | 1424916612-744-4-git-send-email-eyal.birger@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2015-02-26 at 04:10 +0200, Eyal Birger wrote: > Commit 3b885787ea4112 ("net: Generalize socket rx gap / receive queue overflow cmsg") > allowed receiving packet dropcount information as a socket level option. > RXRPC sockets recvmsg function was changed to support this by calling > sock_recv_ts_and_drops() instead of sock_recv_timestamp(). > > However, protocol families wishing to receive dropcount should call > sock_queue_rcv_skb() or set the dropcount specifically (as done > in packet_rcv()). This was not done for rxrpc and thus this feature > never worked on these sockets. > > Formalizing this by not calling sock_recv_ts_and_drops() in rxrpc as > part of an effort to move skb->dropcount into skb->cb[] > > Signed-off-by: Eyal Birger <eyal.birger@gmail.com> > --- > net/rxrpc/ar-recvmsg.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/rxrpc/ar-recvmsg.c b/net/rxrpc/ar-recvmsg.c > index 4575485..d58ba70 100644 > --- a/net/rxrpc/ar-recvmsg.c > +++ b/net/rxrpc/ar-recvmsg.c > @@ -150,7 +150,7 @@ int rxrpc_recvmsg(struct kiocb *iocb, struct socket *sock, > &call->conn->trans->peer->srx, len); > msg->msg_namelen = len; > } > - sock_recv_ts_and_drops(msg, &rx->sk, skb); > + sock_recv_timestamp(msg, &rx->sk, skb); > } I guess this should be the last patch of the serie, as this will overwrite skb->mark, and rxrpc seems to care about skb->mark -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> However, protocol families wishing to receive dropcount should call >> sock_queue_rcv_skb() or set the dropcount specifically (as done >> in packet_rcv()). This was not done for rxrpc and thus this feature >> never worked on these sockets. >> >> Formalizing this by not calling sock_recv_ts_and_drops() in rxrpc as >> part of an effort to move skb->dropcount into skb->cb[] >> >> Signed-off-by: Eyal Birger <eyal.birger@gmail.com> >> --- >> net/rxrpc/ar-recvmsg.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/rxrpc/ar-recvmsg.c b/net/rxrpc/ar-recvmsg.c >> index 4575485..d58ba70 100644 >> --- a/net/rxrpc/ar-recvmsg.c >> +++ b/net/rxrpc/ar-recvmsg.c >> @@ -150,7 +150,7 @@ int rxrpc_recvmsg(struct kiocb *iocb, struct socket *sock, >> &call->conn->trans->peer->srx, len); >> msg->msg_namelen = len; >> } >> - sock_recv_ts_and_drops(msg, &rx->sk, skb); >> + sock_recv_timestamp(msg, &rx->sk, skb); >> } > > I guess this should be the last patch of the serie, as this will > overwrite skb->mark, and rxrpc seems to care about skb->mark > Maybe I didn't understand your comment. Note this patch does not fix SO_RXQ_OVFL on rxrpc sockets. I essentially reverted 3b885787ea4112 for these sockets. It may be possible to compact rxrpc skb->cb[] usage by tricking the size of the resend_at variable as you suggested. However, since SO_RXQ_OVFL never really worked on these sockets, I limited the patch series scope to moving skb->dropcount; This patch only signals that rxrpc can't support this feature unless some space is freed in its skb->cb[] use. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-02-26 at 09:30 +0200, Eyal Birger wrote: > Maybe I didn't understand your comment. Note this patch does not fix > SO_RXQ_OVFL on rxrpc sockets. > > I essentially reverted 3b885787ea4112 for these sockets. > > It may be possible to compact rxrpc skb->cb[] usage by tricking the size > of the resend_at variable as you suggested. > > However, since SO_RXQ_OVFL never really worked on these sockets, > I limited the patch series scope to moving skb->dropcount; This patch only > signals that rxrpc can't support this feature unless some space is freed in its > skb->cb[] use. > -- My point is : If you stick this patch early in the serie, then skb->mark will be overwritten by sock_recv_timestamp() (to store skb->dropcount) and rxrpc breaks as it actively relies on skb->mark (apparently) We dont care if SO_RXQ_OVFL is broken for rxprpc : Nobody noticed yet. But breaking rxrpc skb->mark early in the serie does not help bisection if needed later. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 26, 2015 at 4:31 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Thu, 2015-02-26 at 09:30 +0200, Eyal Birger wrote: > >> Maybe I didn't understand your comment. Note this patch does not fix >> SO_RXQ_OVFL on rxrpc sockets. >> >> I essentially reverted 3b885787ea4112 for these sockets. >> >> It may be possible to compact rxrpc skb->cb[] usage by tricking the size >> of the resend_at variable as you suggested. >> >> However, since SO_RXQ_OVFL never really worked on these sockets, >> I limited the patch series scope to moving skb->dropcount; This patch only >> signals that rxrpc can't support this feature unless some space is freed in its >> skb->cb[] use. >> -- > > My point is : If you stick this patch early in the serie, then skb->mark > will be overwritten by sock_recv_timestamp() (to store skb->dropcount) > and rxrpc breaks as it actively relies on skb->mark (apparently) > skb->dropcount is stored prior to enqueue in sock_queue_rcv_skb(). This function is not called in rxrpc. sock_recv_timestamp() is called from the recvmsg code, and does not alter skb->dropcount. What am I missing? > We dont care if SO_RXQ_OVFL is broken for rxprpc : Nobody noticed yet. > This was my point in this patch - to make it explicit. When rxrpc called sock_recv_ts_and_drops() it allegedly supported receiving drops, when dropcount was never set. > But breaking rxrpc skb->mark early in the serie does not help bisection > if needed later. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/rxrpc/ar-recvmsg.c b/net/rxrpc/ar-recvmsg.c index 4575485..d58ba70 100644 --- a/net/rxrpc/ar-recvmsg.c +++ b/net/rxrpc/ar-recvmsg.c @@ -150,7 +150,7 @@ int rxrpc_recvmsg(struct kiocb *iocb, struct socket *sock, &call->conn->trans->peer->srx, len); msg->msg_namelen = len; } - sock_recv_ts_and_drops(msg, &rx->sk, skb); + sock_recv_timestamp(msg, &rx->sk, skb); } /* receive the message */
Commit 3b885787ea4112 ("net: Generalize socket rx gap / receive queue overflow cmsg") allowed receiving packet dropcount information as a socket level option. RXRPC sockets recvmsg function was changed to support this by calling sock_recv_ts_and_drops() instead of sock_recv_timestamp(). However, protocol families wishing to receive dropcount should call sock_queue_rcv_skb() or set the dropcount specifically (as done in packet_rcv()). This was not done for rxrpc and thus this feature never worked on these sockets. Formalizing this by not calling sock_recv_ts_and_drops() in rxrpc as part of an effort to move skb->dropcount into skb->cb[] Signed-off-by: Eyal Birger <eyal.birger@gmail.com> --- net/rxrpc/ar-recvmsg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)