From: Menglong Dong <imagedong@tencent.com>

In the 1th patch, we do some code cleanup with replease 'sock->sk'
with 'sk'. In the 2th patch, we add statistics for mptcp socket in
use. In the 3th patch, we make mptcp_connect can exit when receive
'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
for this commit.

With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
I belive that the testing of diag.sh can pass now. In fallback and
simultaneous close case, the msk can't release normal (sometimes?) without
that commit, and makes the testing fail. Enn...let's just see that the
CI is saying~


Changes since v6:
- check all processes exit in flush_pids() in the 4th patch

Changes since v5:
- introduce MPTCP_INUSE flag to store if msk is in use, as I find
  that it's not correct to check is a msk is in use by
  !sk_unhashed(sk) in mptcp_destroy_common(), because the token
  can be release in mptcp_check_fastclose()
- add the 3th patch
- reuse __chk_nr in 4th patch

Changes since v4:
- rebase to solve merge conflict

Changes since v3:
- rename MPTCP_DESTROIED to MPTCP_DESTROYED in the 2th patch

Changes since v2:
- add testing

Changes since v1:
- split the code cleanup into the 1th patch.
- decrease the statistics for listening mptcp socket inuse with
  mptcp_listen_inuse_dec()
- add MPTCP_DESTROIED flags to store if mptcp_destroy_common() was
  called on the msk. For fallback case, we need to decrease the
  statistics only once, and mptcp_destroy_common() can be called
  more than once.

Menglong Dong (4):
  mptcp: introduce 'sk' to replace 'sock->sk' in mptcp_listen()
  mptcp: add statistics for mptcp socket in use
  selftest: mptcp: exit from copyfd_io_poll() when receive SIGUSR1
  selftest: mptcp: add test for mptcp socket in use

 net/mptcp/protocol.c                          | 21 +++++---
 net/mptcp/protocol.h                          | 13 +++++
 net/mptcp/subflow.c                           |  1 +
 tools/testing/selftests/net/mptcp/diag.sh     | 54 +++++++++++++++++--
 .../selftests/net/mptcp/mptcp_connect.c       |  4 +-
 5 files changed, 79 insertions(+), 14 deletions(-)

-- 
2.37.2

Hi Menglong Dong,

On 22/11/2022 04:49, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> In the 1th patch, we do some code cleanup with replease 'sock->sk'
> with 'sk'. In the 2th patch, we add statistics for mptcp socket in
> use. In the 3th patch, we make mptcp_connect can exit when receive
> 'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
> for this commit.
> 
> With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
> I belive that the testing of diag.sh can pass now. In fallback and
> simultaneous close case, the msk can't release normal (sometimes?) without
> that commit, and makes the testing fail. Enn...let's just see that the
> CI is saying~
> 
> 
> Changes since v6:
> - check all processes exit in flush_pids() in the 4th patch

Thank you for the new version!

We discussed about this series at the last weekly meeting we had and
globally, it looks good.

However, Paolo mentioned that it is a bit of a shame we need to increase
the complexity of the code for the stats. I agree with you but during
the meeting, we didn't find obvious better ways to avoid having to add a
new flag. I will let a bit more time for Paolo just in case he has
another brilliant idea :)

Other than that, I have some small suggestions for the patch 4/4 but I
can do the modifications when applying the patches later if no other
modifications are needed.

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

Hello,

On Tue, Nov 29, 2022 at 12:05 AM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong Dong,
>
> On 22/11/2022 04:49, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > In the 1th patch, we do some code cleanup with replease 'sock->sk'
> > with 'sk'. In the 2th patch, we add statistics for mptcp socket in
> > use. In the 3th patch, we make mptcp_connect can exit when receive
> > 'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
> > for this commit.
> >
> > With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
> > I belive that the testing of diag.sh can pass now. In fallback and
> > simultaneous close case, the msk can't release normal (sometimes?) without
> > that commit, and makes the testing fail. Enn...let's just see that the
> > CI is saying~
> >
> >
> > Changes since v6:
> > - check all processes exit in flush_pids() in the 4th patch
>
> Thank you for the new version!
>
> We discussed about this series at the last weekly meeting we had and
> globally, it looks good.
>
> However, Paolo mentioned that it is a bit of a shame we need to increase
> the complexity of the code for the stats. I agree with you but during
> the meeting, we didn't find obvious better ways to avoid having to add a
> new flag. I will let a bit more time for Paolo just in case he has
> another brilliant idea :)
>

Thanks for your discussion on this topic! It is indeed not a good
idea to add a new flag for such a case. Looking forward to having
a better solution from Paolo.

(BTW, it's all right to reject this series if it's not good enough :/ )

> Other than that, I have some small suggestions for the patch 4/4 but I
> can do the modifications when applying the patches later if no other
> modifications are needed.
>

Yeah, thanks! I can send a v8 too if you prefer :/

Thanks!
Menglong Dong

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

Hi,

On 30/11/2022 03:11, Menglong Dong wrote:
> Hello,
> 
> On Tue, Nov 29, 2022 at 12:05 AM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Menglong Dong,
>>
>> On 22/11/2022 04:49, menglong8.dong@gmail.com wrote:
>>> From: Menglong Dong <imagedong@tencent.com>
>>>
>>> In the 1th patch, we do some code cleanup with replease 'sock->sk'
>>> with 'sk'. In the 2th patch, we add statistics for mptcp socket in
>>> use. In the 3th patch, we make mptcp_connect can exit when receive
>>> 'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
>>> for this commit.
>>>
>>> With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
>>> I belive that the testing of diag.sh can pass now. In fallback and
>>> simultaneous close case, the msk can't release normal (sometimes?) without
>>> that commit, and makes the testing fail. Enn...let's just see that the
>>> CI is saying~
>>>
>>>
>>> Changes since v6:
>>> - check all processes exit in flush_pids() in the 4th patch
>>
>> Thank you for the new version!
>>
>> We discussed about this series at the last weekly meeting we had and
>> globally, it looks good.
>>
>> However, Paolo mentioned that it is a bit of a shame we need to increase
>> the complexity of the code for the stats. I agree with you but during
>> the meeting, we didn't find obvious better ways to avoid having to add a
>> new flag. I will let a bit more time for Paolo just in case he has
>> another brilliant idea :)
>>
> 
> Thanks for your discussion on this topic! It is indeed not a good
> idea to add a new flag for such a case. Looking forward to having
> a better solution from Paolo.
> 
> (BTW, it's all right to reject this series if it's not good enough :/ )

These stats can be very useful to monitor and debug stuffs around MPTCP!
But yes, if there is a "cleaner" way to do so, that would be better.

Let's not rush things and think a bit more about how this could be done.

>> Other than that, I have some small suggestions for the patch 4/4 but I
>> can do the modifications when applying the patches later if no other
>> modifications are needed.
>>
> 
> Yeah, thanks! I can send a v8 too if you prefer :/

Thank you but for the moment, that's fine. We will probably talk about
this series at the meeting tomorrow.

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

Hi Menglong,

On 22/11/2022 04:49, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> In the 1th patch, we do some code cleanup with replease 'sock->sk'
> with 'sk'. In the 2th patch, we add statistics for mptcp socket in
> use. In the 3th patch, we make mptcp_connect can exit when receive
> 'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
> for this commit.
> 
> With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
> I belive that the testing of diag.sh can pass now. In fallback and
> simultaneous close case, the msk can't release normal (sometimes?) without
> that commit, and makes the testing fail. Enn...let's just see that the
> CI is saying~

Thank you for this new version!
I was waiting the feedback from the CI to start my review but it looks
like the CI was not able to apply your series [1].

I just applied the series manually, fixed the two conflicts I got and
sent it to our repo[2]. We should get the results later on.

Cheers,
Matt

[1]
https://patchew.org/MPTCP/20221122034923.804955-1-imagedong@tencent.com/logs/git/
[2]
https://github.com/multipath-tcp/mptcp_net-next/tree/patchew/20221122034923.804955-1-imagedong@tencent.com
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Hello,

On Wed, Nov 23, 2022 at 11:58 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Menglong,
>
> On 22/11/2022 04:49, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > In the 1th patch, we do some code cleanup with replease 'sock->sk'
> > with 'sk'. In the 2th patch, we add statistics for mptcp socket in
> > use. In the 3th patch, we make mptcp_connect can exit when receive
> > 'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
> > for this commit.
> >
> > With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
> > I belive that the testing of diag.sh can pass now. In fallback and
> > simultaneous close case, the msk can't release normal (sometimes?) without
> > that commit, and makes the testing fail. Enn...let's just see that the
> > CI is saying~
>
> Thank you for this new version!
> I was waiting the feedback from the CI to start my review but it looks
> like the CI was not able to apply your series [1].
>
> I just applied the series manually, fixed the two conflicts I got and
> sent it to our repo[2]. We should get the results later on.
>

Thank you for what you do :/
Enn....seems net-next is not a good branch for mptcp works,
I'll rebase to mptcp_net-next branch next time.

I notice that the CI contains some failures, which I am not sure
if it is caused by my commit. Could you please have a look?
Here is the summary:

https://api.cirrus-ci.com/v1/artifact/task/6563194622181376/summary/summary.txt

Thanks!
Menglong Dong

> Cheers,
> Matt
>
> [1]
> https://patchew.org/MPTCP/20221122034923.804955-1-imagedong@tencent.com/logs/git/
> [2]
> https://github.com/multipath-tcp/mptcp_net-next/tree/patchew/20221122034923.804955-1-imagedong@tencent.com
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Hi Menglong Dong,

On 24/11/2022 10:06, Menglong Dong wrote:
> Hello,
> 
> On Wed, Nov 23, 2022 at 11:58 PM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Menglong,
>>
>> On 22/11/2022 04:49, menglong8.dong@gmail.com wrote:
>>> From: Menglong Dong <imagedong@tencent.com>
>>>
>>> In the 1th patch, we do some code cleanup with replease 'sock->sk'
>>> with 'sk'. In the 2th patch, we add statistics for mptcp socket in
>>> use. In the 3th patch, we make mptcp_connect can exit when receive
>>> 'SIGUSR1' with '-r' flag. And in the 4th patch, we add the testing
>>> for this commit.
>>>
>>> With the commit e8695e504942("mptcp: don't orphan ssk in mptcp_close()"),
>>> I belive that the testing of diag.sh can pass now. In fallback and
>>> simultaneous close case, the msk can't release normal (sometimes?) without
>>> that commit, and makes the testing fail. Enn...let's just see that the
>>> CI is saying~
>>
>> Thank you for this new version!
>> I was waiting the feedback from the CI to start my review but it looks
>> like the CI was not able to apply your series [1].
>>
>> I just applied the series manually, fixed the two conflicts I got and
>> sent it to our repo[2]. We should get the results later on.
>>
> 
> Thank you for what you do :/
> Enn....seems net-next is not a good branch for mptcp works,
> I'll rebase to mptcp_net-next branch next time.

Yes please. Either 'export' (or export-net) or 'for-review' (or
for-review-net), see:


https://github.com/multipath-tcp/mptcp_net-next/wiki/Git-Branches#which-branch-to-use

> I notice that the CI contains some failures, which I am not sure
> if it is caused by my commit.

Indeed, it doesn't seem to be linked to your modification.

I will try to do a bit more tests on my side just to avoid instabilities
but the last modification you did should help stabilising things.

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