Re: [PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"

[Matthieu Baerts <matthieu.baerts@tessares.net>]

Hi Geliang,

On 05/01/2022 11:55, Geliang Tang wrote:
> On Wed, Jan 05, 2022 at 11:01:59AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 05/01/2022 09:50, Geliang Tang wrote:
>>> On Thu, Dec 30, 2021 at 08:16:48PM +0100, Matthieu Baerts wrote:
>>>> From: Geliang Tang <geliang.tang@suse.com>
>>>>
>>>> MP_FAIL could be sent with RST or DSS, and FASTCLOSE can be sent with
>>>> RST too. So we should use a similar xmit logic for FASTCLOSE and
>>>> MP_FAIL in both mptcp_write_options() and mptcp_established_options*().
>>>>
>>>> Cc: Paolo Abeni <pabeni@redhat.com>
>>>> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>>> ---
>>>>  net/mptcp/options.c | 66 ++++++++++++++++++++++++++++-----------------
>>>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index c6726e8389ec..9d2c1c9edbe6 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>>>>  
>>>>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
>>>>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
>>>> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
>>>> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>>>> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
>>>> +			*size += opt_size;
>>>> +			remaining -= opt_size;
>>>> +		}
>>>> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
>>>> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>>>>  			*size += opt_size;
>>>>  			remaining -= opt_size;
>>>>  		}
>>>> @@ -1257,21 +1261,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
>>>>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>  			 struct mptcp_out_options *opts)
>>>>  {
>>>> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>>>> -		const struct sock *ssk = (const struct sock *)tp;
>>>> -		struct mptcp_subflow_context *subflow;
>>>> -
>>>> -		subflow = mptcp_subflow_ctx(ssk);
>>>> -		subflow->send_mp_fail = 0;
>>>> -
>>>> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>>>> -				      TCPOLEN_MPTCP_FAIL,
>>>> -				      0, 0);
>>>> -		put_unaligned_be64(opts->fail_seq, ptr);
>>>> -		ptr += 2;
>>>> -	}
>>>
>>> Hi Matt,
>>>
>>> Sorry, this patch doesn't work on my test. We write both DSS + MP_FAIL,
>>> but MP_FAIL is lost on the received side. Only DSS is received.
>>>
>>> If we write MP_FAIL + DSS like the orignal code, MP_FAIL and DSS will be
>>> received correctly.
>>>
>>> I haven't figured out why yet.
>>>
>>> Could we just keep this MP_FAIL writting code still at the beginning of
>>> the function just like the orignal code did?
>>
>> Thank you for having tested and reviewed the series!
>>
>> Is this issue here fixed by your patch you sent a couple of minutes ago
>> (mptcp: fix a DSS option writting error)
>>
>> That would make sense as the only thing I did was to send the MP_FAIL
>> after the DSS option.
>>
>> Do you mind if I send a v8 series with your new fix the missing 'ptr +=
>> 2;' below and have the code moving the writing of the MP_FAIL option
>> later in a dedicated commit?
> 
> Hi Matt,
> 
> I think about it again. How about just using the v2 of this patch
> (https://patchwork.kernel.org/project/mptcp/patch/5a8c6024481d6106c662c3e892ae5e499b4a7f76.1638156809.git.geliang.tang@suse.com/)
> as a squash-to patch (We have reached an agreement on this part), and move
> all the other code in this v7 as a new patch (It still needs to be
> improved)?

Yes, I was thinking about something similar except that we also need to
modify mptcp_write_options() to allow sending a RST and a FC together.

I just made the split and for this afternoon, I'm hoping to launch the
new tests you added and try to understand the issue. But if you prefer I
already sent the v8 I have, I can.

Thank you for the instructions from your previous email!

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