Re: [PATCH mptcp-next 1/3] Squash to "mptcp: infinite mapping sending"

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Tue, 25 Jan 2022, Geliang Tang wrote:

> Add a flag infinite_sent in struct mptcp_subflow_context, to make sure
> only one infinite map is sent.

Could you explain more about the extra infinite mappings you mention in 
the cover letter? Maybe share a pcap?

>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/options.c  | 2 +-
> net/mptcp/pm.c       | 4 +++-
> net/mptcp/protocol.h | 9 +++++++--
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 0d0d2eb8c8ca..03c82985dba1 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -826,7 +826,7 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>
> 	opts->suboptions = 0;
>
> -	if (unlikely(__mptcp_check_fallback(msk) && !mptcp_check_infinite_map(skb)))
> +	if (unlikely(__mptcp_check_fallback(msk) && !mptcp_check_infinite_map(subflow, skb)))
> 		return false;
>
> 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 1f8878cc29e3..88f0fec1bee5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -275,8 +275,10 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
>
> 	pr_debug("fail_seq=%llu", fail_seq);
>
> -	if (!mptcp_has_another_subflow(sk) && READ_ONCE(msk->allow_infinite_fallback))
> +	if (!mptcp_has_another_subflow(sk) && READ_ONCE(msk->allow_infinite_fallback)) {
> 		subflow->send_infinite_map = 1;
> +		subflow->infinite_sent = 0;
> +	}
> }
>
> /* path manager helpers */
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index c47d69a42fcb..6bcf6cbded45 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -450,6 +450,7 @@ struct mptcp_subflow_context {
> 		send_mp_fail : 1,
> 		send_fastclose : 1,
> 		send_infinite_map : 1,
> +		infinite_sent : 1,
> 		rx_eof : 1,
> 		can_ack : 1,        /* only after processing the remote a key */
> 		disposable : 1,	    /* ctx can be free at ulp release time */
> @@ -893,13 +894,17 @@ static inline void mptcp_do_fallback(struct sock *sk)
>
> #define pr_fallback(a) pr_debug("%s:fallback to TCP (msk=%p)", __func__, a)
>
> -static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
> +static inline bool mptcp_check_infinite_map(struct mptcp_subflow_context *subflow,
> +					    struct sk_buff *skb)
> {
> 	struct mptcp_ext *mpext;
>
> 	mpext = skb ? mptcp_get_ext(skb) : NULL;
> -	if (mpext && mpext->infinite_map)
> +	if (mpext && mpext->infinite_map &&
> +	    !subflow->infinite_sent) {
> +		subflow->infinite_sent = 1;

We don't want to have side effects in this function.

Even though the check for NULL skb means that infinite_sent is not 
modified when tcp_established_options() is called from tcp_current_mss(), 
this doesn't seem right. If this flag is needed (I'm not sure of that 
yet), wouldn't mptcp_update_infinite_map() be a better place for it?

> 		return true;
> +	}
>
> 	return false;
> }
> -- 
> 2.31.1
>
>
>

--
Mat Martineau
Intel