The MPTCP data path is quite complex and hard to understend even
without some foggy comments referring to modified code and/or
completely misleading from the beginning.

Update a few of them to more accurately describing the current
status.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c99eb4ce948e..8feb684408f7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -662,9 +662,9 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
 
 		skb = skb_peek(&ssk->sk_receive_queue);
 		if (!skb) {
-			/* if no data is found, a racing workqueue/recvmsg
-			 * already processed the new data, stop here or we
-			 * can enter an infinite loop
+			/* With racing move_skbs_to_msk() and __mptcp_move_skbs(),
+			 * a different CPU can have already processed the pending
+			 * data, stop here or we can enter an infinite loop
 			 */
 			if (!moved)
 				done = true;
@@ -672,9 +672,9 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
 		}
 
 		if (__mptcp_check_fallback(msk)) {
-			/* if we are running under the workqueue, TCP could have
-			 * collapsed skbs between dummy map creation and now
-			 * be sure to adjust the size
+			/* Under fallback skbs have no MPTCP extension and TCP could
+			 * collapse them between the dummy map creation and the
+			 * current dequeue. Be sure to adjust the map size.
 			 */
 			map_remaining = skb->len;
 			subflow->map_data_len = skb->len;
@@ -3767,7 +3767,7 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
 	if (sk->sk_shutdown & RCV_SHUTDOWN)
 		mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
 
-	/* This barrier is coupled with smp_wmb() in tcp_reset() */
+	/* This barrier is coupled with smp_wmb() in __mptcp_error_report() */
 	smp_rmb();
 	if (sk->sk_err)
 		mask |= EPOLLERR;
-- 
2.37.3

Hi Paolo, Mat,

On 26/09/2022 17:27, Paolo Abeni wrote:
> The MPTCP data path is quite complex and hard to understend even
> without some foggy comments referring to modified code and/or
> completely misleading from the beginning.
> 
> Update a few of them to more accurately describing the current
> status.

Good idea, thank you for the patch and the review!

Now in our tree (feat. for net-next) with Mat's RvB tag:

New patches for t/upstream:
- 2bf3d5448679: mptcp: update misleading comments.
- Results: 676709a1f80b..34c4fe0fbc95 (export)


Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220927T075707

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Mon, 26 Sep 2022, Paolo Abeni wrote:

> The MPTCP data path is quite complex and hard to understend even
> without some foggy comments referring to modified code and/or
> completely misleading from the beginning.
>
> Update a few of them to more accurately describing the current
> status.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thanks for updating the comments, Paolo. New comments look good to me:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>


> ---
> net/mptcp/protocol.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c99eb4ce948e..8feb684408f7 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -662,9 +662,9 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
>
> 		skb = skb_peek(&ssk->sk_receive_queue);
> 		if (!skb) {
> -			/* if no data is found, a racing workqueue/recvmsg
> -			 * already processed the new data, stop here or we
> -			 * can enter an infinite loop
> +			/* With racing move_skbs_to_msk() and __mptcp_move_skbs(),
> +			 * a different CPU can have already processed the pending
> +			 * data, stop here or we can enter an infinite loop
> 			 */
> 			if (!moved)
> 				done = true;
> @@ -672,9 +672,9 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
> 		}
>
> 		if (__mptcp_check_fallback(msk)) {
> -			/* if we are running under the workqueue, TCP could have
> -			 * collapsed skbs between dummy map creation and now
> -			 * be sure to adjust the size
> +			/* Under fallback skbs have no MPTCP extension and TCP could
> +			 * collapse them between the dummy map creation and the
> +			 * current dequeue. Be sure to adjust the map size.
> 			 */
> 			map_remaining = skb->len;
> 			subflow->map_data_len = skb->len;
> @@ -3767,7 +3767,7 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
> 	if (sk->sk_shutdown & RCV_SHUTDOWN)
> 		mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
>
> -	/* This barrier is coupled with smp_wmb() in tcp_reset() */
> +	/* This barrier is coupled with smp_wmb() in __mptcp_error_report() */
> 	smp_rmb();
> 	if (sk->sk_err)
> 		mask |= EPOLLERR;
> -- 
> 2.37.3
>
>
>

--
Mat Martineau
Intel