net/mptcp/options.c | 105 ++++++++++++----- tools/testing/selftests/net/mptcp/config | 5 + .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++-- 3 files changed, 182 insertions(+), 39 deletions(-)
This is a new version of Geliang's series called "send MP_FAIL with MP_RST and
others" (v6 + v7) but also include this time:
- "mptcp: fix a DSS option writting error" (v1)
- "selftests: mptcp: add mp_fail testcases" (v5)
On top of these two new patches from Geliang already sent on the ML, this new
version:
- includes one new patch 2/8 ("mptcp: fix opt size when sending DSS + MP_FAIL"):
hopefuly fixing the issue reported by Geliang (MPF+DSS ok, DSS+MPF nok)
- has the Squash-to patch 1/8 split: the other part is in patch 4/8
("mptcp: reduce branching when writing MP_FAIL option")
- has one fix in patch 1/8 reported by Geliang
Notes: I still have issues with the mptcp_join selftests and TC but this can be
fixed later.
Geliang Tang (4):
Squash to "mptcp: implement fastclose xmit path"
mptcp: fix a DSS option writing error
mptcp: print out reset infos of MP_RST
selftests: mptcp: add mp_fail testcases
Matthieu Baerts (4):
mptcp: fix opt size when sending DSS + MP_FAIL
mptcp: reduce branching when writing MP_FAIL option
mptcp: clarify when options can be used
mptcp: allow sending both ADD_ADDR and RM_ADDR
net/mptcp/options.c | 105 ++++++++++++-----
tools/testing/selftests/net/mptcp/config | 5 +
.../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
3 files changed, 182 insertions(+), 39 deletions(-)
--
2.33.1
Hi Matt,
Thank you so much for this new version. Good catch on patch 2. It works on
my test and all looks good to me...
Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
... except patch 6.
I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
better not to let ADD_ADDR sent with others by ourselves.
On the other hand, I'm not sure our code can deal with ADD_ADDR and RM_ADDR
together correctly, it's not tested.
WDYT?
Thanks,
-Geliang
Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月5日周三 23:57写道：
>
> This is a new version of Geliang's series called "send MP_FAIL with MP_RST and
> others" (v6 + v7) but also include this time:
>
> - "mptcp: fix a DSS option writting error" (v1)
> - "selftests: mptcp: add mp_fail testcases" (v5)
>
> On top of these two new patches from Geliang already sent on the ML, this new
> version:
>
> - includes one new patch 2/8 ("mptcp: fix opt size when sending DSS + MP_FAIL"):
> hopefuly fixing the issue reported by Geliang (MPF+DSS ok, DSS+MPF nok)
> - has the Squash-to patch 1/8 split: the other part is in patch 4/8
> ("mptcp: reduce branching when writing MP_FAIL option")
> - has one fix in patch 1/8 reported by Geliang
>
> Notes: I still have issues with the mptcp_join selftests and TC but this can be
> fixed later.
>
> Geliang Tang (4):
> Squash to "mptcp: implement fastclose xmit path"
> mptcp: fix a DSS option writing error
> mptcp: print out reset infos of MP_RST
> selftests: mptcp: add mp_fail testcases
>
> Matthieu Baerts (4):
> mptcp: fix opt size when sending DSS + MP_FAIL
> mptcp: reduce branching when writing MP_FAIL option
> mptcp: clarify when options can be used
> mptcp: allow sending both ADD_ADDR and RM_ADDR
>
> net/mptcp/options.c | 105 ++++++++++++-----
> tools/testing/selftests/net/mptcp/config | 5 +
> .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
> 3 files changed, 182 insertions(+), 39 deletions(-)
>
> --
> 2.33.1
>
>
On Thu, 2022-01-06 at 10:58 +0800, Geliang Tang wrote:
> Hi Matt,
>
> Thank you so much for this new version. Good catch on patch 2. It works on
> my test and all looks good to me...
>
> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
>
> ... except patch 6.
>
> I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
> efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
> better not to let ADD_ADDR sent with others by ourselves.
+1
/P
Hi Geliang,
On 06/01/2022 03:58, Geliang Tang wrote:
> Hi Matt,
>
> Thank you so much for this new version. Good catch on patch 2. It works on
> my test and all looks good to me...
>
> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
Great, thank you for having checked that!
> ... except patch 6.
>
> I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
> efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
> better not to let ADD_ADDR sent with others by ourselves.
ADD_ADDR suboption can be too big in some cases but not all the time.
If I remember well, we decided to have a dedicated ACK to simplify the
code: not having the ADD_ADDR removing the DSS options in some cases but
not all, also simplifying packetdrill tests.
Here, the idea is to add the RM_ADDR if asked and if there is room, just
like what we do in mptcp_write_options().
> On the other hand, I'm not sure our code can deal with ADD_ADDR and RM_ADDR
> together correctly, it's not tested.
Good point, we need a test validating that.
I'm not even sure we can have a test because each netlink command will
cause the corresponding option to be sent: depending on the timing, when
the next packet is sent, we might have one or two options to be sent.
If we drop patch 6, we need to replace it with a patch updating the new
comment at the beginning of mptcp_write_options() and not allowing
ADD_ADDR to be sent with RM_ADDR + MP_PRIO as we currently do.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
On Thu, 6 Jan 2022, Matthieu Baerts wrote:
> Hi Geliang,
>
> On 06/01/2022 03:58, Geliang Tang wrote:
>> Hi Matt,
>>
>> Thank you so much for this new version. Good catch on patch 2. It works on
>> my test and all looks good to me...
>>
>> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
>
> Great, thank you for having checked that!
>
>> ... except patch 6.
>>
>> I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
>> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
>> efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
>> better not to let ADD_ADDR sent with others by ourselves.
>
> ADD_ADDR suboption can be too big in some cases but not all the time.
> If I remember well, we decided to have a dedicated ACK to simplify the
> code: not having the ADD_ADDR removing the DSS options in some cases but
> not all, also simplifying packetdrill tests.
>
> Here, the idea is to add the RM_ADDR if asked and if there is room, just
> like what we do in mptcp_write_options().
>
>> On the other hand, I'm not sure our code can deal with ADD_ADDR and RM_ADDR
>> together correctly, it's not tested.
>
> Good point, we need a test validating that.
>
Sounds like a job for packetdrill!
> I'm not even sure we can have a test because each netlink command will
> cause the corresponding option to be sent: depending on the timing, when
> the next packet is sent, we might have one or two options to be sent.
>
> If we drop patch 6, we need to replace it with a patch updating the new
> comment at the beginning of mptcp_write_options() and not allowing
> ADD_ADDR to be sent with RM_ADDR + MP_PRIO as we currently do.
>
I think it would be good to drop patch 6 and updating the comment as
Matthieu says.
I'm still seeing the file content mismatch with the "MP_FAIL, multiple
subflows" test, which doesn't seem right. Will try to look into what's
going on there next week.
--
Mat Martineau
Intel
On 08/01/2022 02:19, Mat Martineau wrote:
> On Thu, 6 Jan 2022, Matthieu Baerts wrote:
>
>> Hi Geliang,
>>
>> On 06/01/2022 03:58, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thank you so much for this new version. Good catch on patch 2. It
>>> works on
>>> my test and all looks good to me...
>>>
>>> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
>>
>> Great, thank you for having checked that!
>>
>>> ... except patch 6.
>>>
>>> I thinks we'd better to drop patch 6 ("mptcp: allow sending both
>>> ADD_ADDR
>>> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
>>> efforts to let ADD_ADDR not to be sent with other subopthins. I
>>> thinks it's
>>> better not to let ADD_ADDR sent with others by ourselves.
>>
>> ADD_ADDR suboption can be too big in some cases but not all the time.
>> If I remember well, we decided to have a dedicated ACK to simplify the
>> code: not having the ADD_ADDR removing the DSS options in some cases but
>> not all, also simplifying packetdrill tests.
>>
>> Here, the idea is to add the RM_ADDR if asked and if there is room, just
>> like what we do in mptcp_write_options().
>>
>>> On the other hand, I'm not sure our code can deal with ADD_ADDR and
>>> RM_ADDR
>>> together correctly, it's not tested.
>>
>> Good point, we need a test validating that.
>>
>
> Sounds like a job for packetdrill!
Yes but still, we need a netlink command to ask the kernel sending an
ADD_ADDR/RM_ADDR/MP_PRIO, one at a time.
I don't think we can have a proper test always making the kernel sending
a packet with two notifications, no?
>> I'm not even sure we can have a test because each netlink command will
>> cause the corresponding option to be sent: depending on the timing, when
>> the next packet is sent, we might have one or two options to be sent.
>>
>> If we drop patch 6, we need to replace it with a patch updating the new
>> comment at the beginning of mptcp_write_options() and not allowing
>> ADD_ADDR to be sent with RM_ADDR + MP_PRIO as we currently do.
>>
>
> I think it would be good to drop patch 6 and updating the comment as
> Matthieu says.
I think we should also modify the code in "mptcp_write_options()" and in
"mptcp_established_options()" to reflect that.
Maybe it would simplify things to say: we can send an RM_ADDR and
MP_PRIO only with a DSS, one at a time. That we can validate (we already
do) and I don't think we will need to send in the same TCP packet an
ADD_ADDR + RM_ADDR + MP_PRIO.
That would replace a lot of "C" (can) by "P" (prefer not) in the
previous comment and reduce cases to manage.
WDYT?
>
> I'm still seeing the file content mismatch with the "MP_FAIL, multiple
> subflows" test, which doesn't seem right. Will try to look into what's
> going on there next week.
Same here.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Hi Geliang, Mat, On 06/01/2022 03:58, Geliang Tang wrote: > Hi Matt, > > Thank you so much for this new version. Good catch on patch 2. It works on > my test and all looks good to me... > > Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com> As discussed in another thread on the ML, I just applied the 3 first patches: - 62aa763b1676: "squashed" patch 1/8 in "mptcp: implement fastclose xmit path" - 5c6f43e1c2c0: "Signed-off-by" + "Co-developed-by" - Results: 720eaaa70083..02e9b8087979 - 55c56c19c276: mptcp: fix opt size when sending DSS + MP_FAIL - 025c1fcd8fa3: mptcp: fix a DSS option writing error (- 73463b55be2a: mptcp: Check reclaim amount before reducing allocation) - Results: 02e9b8087979..d500488d62db Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220106T114429 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
On Thu, 6 Jan 2022, Matthieu Baerts wrote: > Hi Geliang, Mat, > > On 06/01/2022 03:58, Geliang Tang wrote: >> Hi Matt, >> >> Thank you so much for this new version. Good catch on patch 2. It works on >> my test and all looks good to me... >> >> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com> > > As discussed in another thread on the ML, I just applied the 3 first > patches: > > - 62aa763b1676: "squashed" patch 1/8 in "mptcp: implement fastclose xmit > path" > - 5c6f43e1c2c0: "Signed-off-by" + "Co-developed-by" > - Results: 720eaaa70083..02e9b8087979 > > - 55c56c19c276: mptcp: fix opt size when sending DSS + MP_FAIL > - 025c1fcd8fa3: mptcp: fix a DSS option writing error > (- 73463b55be2a: mptcp: Check reclaim amount before reducing allocation) > - Results: 02e9b8087979..d500488d62db > Thanks for applying these to the export branch so they could get tested (results look ok), that will help with getting them upstream this week. > Builds and tests are now in progress: > > https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220106T114429 > https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export -- Mat Martineau Intel
© 2016 - 2024 Red Hat, Inc.