Message ID | 1618298656-123756-1-git-send-email-liyonglong@chinatelecom.cn |
---|---|
State | Superseded, archived |
Delegated to: | Paolo Abeni |
Headers | show |
Series | mptcp: add MSG_PEEK support | expand |
Hello, On Tue, 2021-04-13 at 15:24 +0800, Yonglong Li wrote: > This patch adds support for MSG_PEEK flag. Packets are not removed > from the receive_queue if MSG_PEEK set in recv() system call. > > Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn> > --- > net/mptcp/protocol.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 2d895c3..65448e1 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1710,12 +1710,13 @@ static void mptcp_wait_data(struct sock *sk, long *timeo) > > static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk, > struct msghdr *msg, > - size_t len) > + size_t len, int flags) > { > struct sk_buff *skb; > int copied = 0; > > - while ((skb = skb_peek(&msk->receive_queue)) != NULL) { > + skb = skb_peek(&msk->receive_queue); > + while (!(skb == NULL || skb == (struct sk_buff *)(&msk->receive_queue))) { I think you can replace the above construct with skb_queue_walk_safe(). Will be cleaner and will avoid the skb_peek()/skb = skb->next later. > u32 offset = MPTCP_SKB_CB(skb)->offset; > u32 data_len = skb->len - offset; > u32 count = min_t(size_t, len - copied, data_len); > @@ -1731,15 +1732,21 @@ static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk, > copied += count; > > if (count < data_len) { > - MPTCP_SKB_CB(skb)->offset += count; > + if (!(flags & MSG_PEEK)) > + MPTCP_SKB_CB(skb)->offset += count; > break; > } > > - /* we will bulk release the skb memory later */ > - skb->destructor = NULL; > - msk->rmem_released += skb->truesize; > - __skb_unlink(skb, &msk->receive_queue); > - __kfree_skb(skb); > + if (!(flags & MSG_PEEK)) > + /* we will bulk release the skb memory later */ It looks like there is a missing bracket here. > + skb->destructor = NULL; > + msk->rmem_released += skb->truesize; > + __skb_unlink(skb, &msk->receive_queue); > + __kfree_skb(skb); > + skb = skb_peek(&msk->receive_queue); > + }else{ Uhm... this causes a compiler error due to missing paired opening bracket. Additional minor hint, a space is needed before and after the 'else' > + skb = skb->next; > + } > > if (copied >= len) > break; > @@ -1933,7 +1940,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > while (copied < len) { > int bytes_read; > > - bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied); > + bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied, flags); > if (unlikely(bytes_read < 0)) { > if (!copied) > copied = bytes_read; > @@ -2017,7 +2024,8 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, > pr_debug("msk=%p data_ready=%d rx queue empty=%d copied=%d", > msk, test_bit(MPTCP_DATA_READY, &msk->flags), > skb_queue_empty_lockless(&sk->sk_receive_queue), copied); > - mptcp_rcv_space_adjust(msk, copied); > + if (!(flags & MSG_PEEK)) > + mptcp_rcv_space_adjust(msk, copied); > > release_sock(sk); > return copied; This lacks some other changes in mptcp_recvmsg(). Currently mptcp_recvmsg() fails with -EOPNOTSUPP if an unsupported flag is provided as an argument, and MSG_PEEK is not yet in the supported list. Side note: we need to change the above: mptcp_recvmsg() should silently ignore unsupported flags. Finally you should add some self-tests for the new feature (in a separate patch). You could extend the 'mptcp_connect.c' program and add some new script in the 'mptcp' self-test directory. That will allow you to test your code, as needed. If you are interested in larger contribution to the MPTCP protocol, I suggest to try to join the public mtg we have every week: https://github.com/multipath-tcp/mptcp_net-next/wiki/Meetings The current timeslot is likely unfortunate, but we could change that. Cheers, Paolo
Hi Paolo, Thanks for your review. embarrassedly... I will prepare a v2 patch as your suggestion. And add some self-tests for the new feature. On 2021/4/13 17:29, Paolo Abeni wrote: > Hello, > > On Tue, 2021-04-13 at 15:24 +0800, Yonglong Li wrote: >> This patch adds support for MSG_PEEK flag. Packets are not removed >> from the receive_queue if MSG_PEEK set in recv() system call. >> >> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn> >> --- >> net/mptcp/protocol.c | 28 ++++++++++++++++++---------- >> 1 file changed, 18 insertions(+), 10 deletions(-) >> >> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >> index 2d895c3..65448e1 100644 >> --- a/net/mptcp/protocol.c >> +++ b/net/mptcp/protocol.c >> @@ -1710,12 +1710,13 @@ static void mptcp_wait_data(struct sock *sk, long *timeo) >> >> static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk, >> struct msghdr *msg, >> - size_t len) >> + size_t len, int flags) >> { >> struct sk_buff *skb; >> int copied = 0; >> >> - while ((skb = skb_peek(&msk->receive_queue)) != NULL) { >> + skb = skb_peek(&msk->receive_queue); >> + while (!(skb == NULL || skb == (struct sk_buff *)(&msk->receive_queue))) { > > I think you can replace the above construct with skb_queue_walk_safe(). > Will be cleaner and will avoid the skb_peek()/skb = skb->next later. Your are right, here replace with skb_queue_walk_safe() will be cleaner. > >> u32 offset = MPTCP_SKB_CB(skb)->offset; >> u32 data_len = skb->len - offset; >> u32 count = min_t(size_t, len - copied, data_len); >> @@ -1731,15 +1732,21 @@ static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk, >> copied += count; >> >> if (count < data_len) { >> - MPTCP_SKB_CB(skb)->offset += count; >> + if (!(flags & MSG_PEEK)) >> + MPTCP_SKB_CB(skb)->offset += count; >> break; >> } >> >> - /* we will bulk release the skb memory later */ >> - skb->destructor = NULL; >> - msk->rmem_released += skb->truesize; >> - __skb_unlink(skb, &msk->receive_queue); >> - __kfree_skb(skb); >> + if (!(flags & MSG_PEEK)) >> + /* we will bulk release the skb memory later */ > > It looks like there is a missing bracket her> >> + skb->destructor = NULL; >> + msk->rmem_released += skb->truesize; >> + __skb_unlink(skb, &msk->receive_queue); >> + __kfree_skb(skb); >> + skb = skb_peek(&msk->receive_queue); >> + }else{ > > Uhm... this causes a compiler error due to missing paired opening > bracket. > > Additional minor hint, a space is needed before and after the 'else' > >> + skb = skb->next; >> + } >> >> if (copied >= len) >> break; >> @@ -1933,7 +1940,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, >> while (copied < len) { >> int bytes_read; >> >> - bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied); >> + bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied, flags); >> if (unlikely(bytes_read < 0)) { >> if (!copied) >> copied = bytes_read; >> @@ -2017,7 +2024,8 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, >> pr_debug("msk=%p data_ready=%d rx queue empty=%d copied=%d", >> msk, test_bit(MPTCP_DATA_READY, &msk->flags), >> skb_queue_empty_lockless(&sk->sk_receive_queue), copied); >> - mptcp_rcv_space_adjust(msk, copied); >> + if (!(flags & MSG_PEEK)) >> + mptcp_rcv_space_adjust(msk, copied); >> >> release_sock(sk); >> return copied; > > This lacks some other changes in mptcp_recvmsg(). > Currently mptcp_recvmsg() fails with -EOPNOTSUPP if an unsupported flag > is provided as an argument, and MSG_PEEK is not yet in the supported > list. > > Side note: we need to change the above: mptcp_recvmsg() should silently > ignore unsupported flags. > > Finally you should add some self-tests for the new feature (in a > separate patch). You could extend the 'mptcp_connect.c' program and add > some new script in the 'mptcp' self-test directory. That will allow you > to test your code, as needed. > > If you are interested in larger contribution to the MPTCP protocol, I > suggest to try to join the public mtg we have every week: > > https://github.com/multipath-tcp/mptcp_net-next/wiki/Meetings > > The current timeslot is likely unfortunate, but we could change that. > > Cheers, > > Paolo > > >
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 2d895c3..65448e1 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1710,12 +1710,13 @@ static void mptcp_wait_data(struct sock *sk, long *timeo) static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk, struct msghdr *msg, - size_t len) + size_t len, int flags) { struct sk_buff *skb; int copied = 0; - while ((skb = skb_peek(&msk->receive_queue)) != NULL) { + skb = skb_peek(&msk->receive_queue); + while (!(skb == NULL || skb == (struct sk_buff *)(&msk->receive_queue))) { u32 offset = MPTCP_SKB_CB(skb)->offset; u32 data_len = skb->len - offset; u32 count = min_t(size_t, len - copied, data_len); @@ -1731,15 +1732,21 @@ static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk, copied += count; if (count < data_len) { - MPTCP_SKB_CB(skb)->offset += count; + if (!(flags & MSG_PEEK)) + MPTCP_SKB_CB(skb)->offset += count; break; } - /* we will bulk release the skb memory later */ - skb->destructor = NULL; - msk->rmem_released += skb->truesize; - __skb_unlink(skb, &msk->receive_queue); - __kfree_skb(skb); + if (!(flags & MSG_PEEK)) + /* we will bulk release the skb memory later */ + skb->destructor = NULL; + msk->rmem_released += skb->truesize; + __skb_unlink(skb, &msk->receive_queue); + __kfree_skb(skb); + skb = skb_peek(&msk->receive_queue); + }else{ + skb = skb->next; + } if (copied >= len) break; @@ -1933,7 +1940,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, while (copied < len) { int bytes_read; - bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied); + bytes_read = __mptcp_recvmsg_mskq(msk, msg, len - copied, flags); if (unlikely(bytes_read < 0)) { if (!copied) copied = bytes_read; @@ -2017,7 +2024,8 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, pr_debug("msk=%p data_ready=%d rx queue empty=%d copied=%d", msk, test_bit(MPTCP_DATA_READY, &msk->flags), skb_queue_empty_lockless(&sk->sk_receive_queue), copied); - mptcp_rcv_space_adjust(msk, copied); + if (!(flags & MSG_PEEK)) + mptcp_rcv_space_adjust(msk, copied); release_sock(sk); return copied;
This patch adds support for MSG_PEEK flag. Packets are not removed from the receive_queue if MSG_PEEK set in recv() system call. Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn> --- net/mptcp/protocol.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)