v14:
 - implement flush operation in user space as Mat suggested.
 - update selftests.
 - Now this series includes five parts:

Part 1: dump for userspace pm (patches 1-9)
Part 2: fixes for creating id 0 subflow (patches 10-13)
Part 3: v4-mapped addr support (patches 14-15)
Part 4: flush for userspace pm (patches 16-18)
Part 5: address entry refcount for userspace pm (patches 19-25)

v13:
 - add 4 patches: 10, 11, 31, 32.
 - update selftests.

v12:
 - add pm_remove_subflows, instead of changing
   pm_remove_addrs_and_subflows.

v11:
 - add a patch "mptcp: userspace pm send RM_ADDR for conn_list addr" to
   fix selftests failures reported by CI.

v10:

 - add "fixes for creating id 0 subflow" part.

v9:
 - Fix typos reported by CI.
 - Squash two patches
        "selftests: mptcp: pm_netlink: print colored output"
        "selftests: mptcp: add mptcp_lib_check helper"
   into one:
        "selftests: mptcp: add mptcp_lib_check helper"

v8:
 - add mptcp_lib_check helper

v7:
 - merge 'Squash to "mptcp: add use_id parameter for addresses_equal
   v6"', fix packetdrill_add_addr error.
 - fix memleak error in "mptcp: add netlink pm addr entry refcount".
 - split "selftests: mptcp: flush and dump userspace addrs list" into
   two patches.

v6:
 - fix kmemleak errors reported by CI.
 - drop a patch "mptcp: add netlink pm addr entry refcount".

v5:
- Put the two series "add flush and dump for userspace" and
 "add refcount for address entry" together for better CI testing.

Patches 1-12: add flush and dump for userspace

v4:
 - fix the deadlock issue in v3 reported by CI.

v3:
 - fix warnings reported by CI.
 - get id_bitmap using pm_nl_get_pernet_from_msk.

v2:
 - add two patches: "mptcp: check userspace pm subflow flag"
                    "selftests: mptcp: add userspace pm subflow flag"

This series adds flush and dump commands support for userspace pm.

Patches 13-21: add refcount for address entry

v4:
 - move two patches here from "add flush and dump for userspace pm":
  mptcp: add userspace_pm_get_entry helper
  mptcp: drop addr_match and id_match

v3:
 - add four selftests patches:
  selftests: mptcp: export event macros in mptcp_lib
  selftests: mptcp: extract mptcp_lib_check_expected
  selftests: mptcp: add mptcp_lib_verify_listener_events
  selftests: mptcp: add mptcp_lib_init_ns

v2:
 - rebased with "add flush and dump for userspace pm" series.

Add refcount for address entry.

Geliang Tang (25):
  mptcp: export pm_nl_get_pernet_from_msk
  mptcp: drop mptcp_pm_get_* helpers
  mptcp: use pernet id_bitmap in userspace pm
  mptcp: add userspace_pm_lookup_addr_by_id helper
  mptcp: drop lookup_by_id parameter in lookup_addr
  mptcp: dump addrs in userspace pm list
  mptcp: check userspace pm subflow flag
  selftests: mptcp: add userspace pm subflow flag
  selftests: mptcp: dump userspace addrs list
  mptcp: set set_id flag when parsing addr
  mptcp: use set_id flag when appending addr
  mptcp: check addrs list in userspace_pm_get_local_id
  selftests: mptcp: dump after creating id 0 subflow
  mptcp: map v4 address to v6 when destroying subflow
  selftests: mptcp: rm subflow with v4/v4mapped addr
  mptcp: make pm_remove_addrs_and_subflows static
  mptcp: add a prefix for free_local_addr_list
  selftests: mptcp: flush userspace addrs list
  mptcp: add use_id parameter for addresses_equal
  mptcp: add check_id for lookup_anno_list_by_saddr
  mptcp: add userspace_pm_get_entry helper
  mptcp: drop addr_match and id_match
  mptcp: dup an entry when removing it
  mptcp: add userspace pm addr entry refcount
  selftests: mptcp: rm userspace addr with random order

 net/mptcp/pm.c                                |  16 +-
 net/mptcp/pm_netlink.c                        | 170 +++++++---------
 net/mptcp/pm_userspace.c                      | 189 ++++++++++++------
 net/mptcp/protocol.c                          |   2 +-
 net/mptcp/protocol.h                          |  42 +++-
 net/mptcp/sockopt.c                           |   9 +-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 128 +++++++++++-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c |   8 +
 8 files changed, 381 insertions(+), 183 deletions(-)

-- 
2.35.3

On Fri, 8 Dec 2023, Geliang Tang wrote:

> Part 1: dump for userspace pm (patches 1-9)
> Part 2: fixes for creating id 0 subflow (patches 10-13)
> Part 3: v4-mapped addr support (patches 14-15)


Hi Geliang -

I think we should focus on parts 1-3 for enhancing the userspace PM. If 
the userspace daemon has to restart, it needs to find out which local 
address IDs have been advertised to the peer so it can avoid attempted 
reuse of those IDs, and it can send REMOVE_ADDR for those IDs if needed.


> Part 4: flush for userspace pm (patches 16-18)

It looks like the flush patches were removed after v13 except for the 
selftest (patch 18?), and it looks like patches 16-17 are not related to 
the flush operation.


> Part 5: address entry refcount for userspace pm (patches 19-25)

These patches are to address 
https://github.com/multipath-tcp/mptcp_net-next/issues/403, correct?

I think a refcount is not necessary to make the userspace local address 
list work correctly.

The userspace local address list should contain:

  * An entry for ID 0 (when the connection is started - it may be removed 
later)

  * An entry for every ID advertised with ADD_ADDR

  * An entry for every ID allocated by the kernel when an outgoing MP_JOIN 
uses an address that doesn't have an existing ID.


Entries should only be deleted by a "remove" netlink command from the 
userspace PM daemon. Closing subflows (or connection errors) should not 
remove entries from the local address list.

Does anyone remember why the current code deletes address ID entries when 
subflows are destroyed or connections fail?

If the local address list is defined as "the 
list of address IDs we have advertised to the peer", there is no need to 
manage the list as subflows are added and removed. Even when no subflows 
currently exist, the peer still can have cached address IDs from our 
device, so they must be considered valid until we send a REMOVE_ADDR.


I suggest modifying the userspace local addr code to make '0' valid. If 
there's a need to track whether the ID is unassigned, maybe struct 
mptcp_addr_info needs a separate flag for that.



Another overall note: I couldn't get the series to fully apply to recent 
export branch tags. Please remember to add a note to the cover letter 
describing what other patch series it depends on, and what order to apply 
them in! (Or give a link to a public git repo/branch)

Hi Mat, Geliang,

Sorry for the delay.

On 09/12/2023 02:15, Mat Martineau wrote:
> On Fri, 8 Dec 2023, Geliang Tang wrote:

(...)

>> Part 5: address entry refcount for userspace pm (patches 19-25)
> 
> These patches are to address
> https://github.com/multipath-tcp/mptcp_net-next/issues/403, correct?
> 
> I think a refcount is not necessary to make the userspace local address
> list work correctly.
> 
> The userspace local address list should contain:
> 
>  * An entry for ID 0 (when the connection is started - it may be removed
> later)
> 
>  * An entry for every ID advertised with ADD_ADDR
> 
>  * An entry for every ID allocated by the kernel when an outgoing
> MP_JOIN uses an address that doesn't have an existing ID.

Yes, I agree with that: it is required to track all explicit (ADD_ADDR)
and implicit (MPC + MPJ) addresses. Technically, we don't need a single
list for that, as long as there is a helper function to look at all
these addresses.

> Entries should only be deleted by a "remove" netlink command from the
> userspace PM daemon. Closing subflows (or connection errors) should not
> remove entries from the local address list.

Correct, only when sending a REMOVE_ADDR. Indeed, the PM might want to
delete a subflow for some reason (e.g. saving bandwidth), but it doesn't
mean the address became unavailable.

> Does anyone remember why the current code deletes address ID entries
> when subflows are destroyed or connections fail?

I wonder if there were not some confusions with pm.local_addr_used
counter. We might want to know the "actual" status, but it is true that
it should show what has been announced, and not what is being used.

For the destroyed, that's indeed not correct: we should then not remove
the entry.

For the connections fails -- not able to queue the SYN+MPJ in
__mptcp_subflow_connect() from mptcp_pm_nl_subflow_create_doit() --,
that's maybe different, no? Nothing has been sent on the wire, maybe the
new address entry should be removed? But for this specific case, we
don't need a refcount, we can add the entry after having called
__mptcp_subflow_connect or we modify
mptcp_userspace_pm_append_new_local_addr() to also tell us if a new
entry has been created.

> If the local address list is defined as "the list of address IDs we have
> advertised to the peer", there is no need to manage the list as subflows
> are added and removed. Even when no subflows currently exist, the peer
> still can have cached address IDs from our device, so they must be
> considered valid until we send a REMOVE_ADDR.

Agreed.

> I suggest modifying the userspace local addr code to make '0' valid. If
> there's a need to track whether the ID is unassigned, maybe struct
> mptcp_addr_info needs a separate flag for that.

Good point, thank you for having spotted that.

So, if I'm not mistaken:

- mptcp_pm_nl_subflow_create_doit() should *not* call
mptcp_userspace_pm_delete_local_addr() or *only* remove the entry if it
has just been created before.

- mptcp_pm_nl_subflow_destroy_doit() should *not* call
mptcp_userspace_pm_delete_local_addr()

- mptcp_pm_nl_remove_doit() should call
mptcp_userspace_pm_delete_local_addr() (which might need to be modified)

- We should replace this comment above
mptcp_userspace_pm_delete_local_addr()...

/* If the subflow is closed from the other peer (not via a
 * subflow destroy command then), we want to keep the entry
 * not to assign the same ID to another address and to be
 * able to send RM_ADDR after the removal of the subflow.
 */

... by something like: Only remove entries from the local addr list if
the address has been explicitly removed via a REMOVE_ADDR. Removing a
subflow doesn't mean the address became unavailable.

- We should remove the TODO about the refcount in
mptcp_userspace_pm_delete_local_addr()

- And close https://github.com/multipath-tcp/mptcp_net-next/issues/403


@Mat / Geliang: Is this correct? WDYT?

@Geliang: is it something you were already looking at?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Hi Geliang,

On 08/12/2023 11:07, Geliang Tang wrote:
> v14:
>  - implement flush operation in user space as Mat suggested.
>  - update selftests.
>  - Now this series includes five parts:
> 
> Part 1: dump for userspace pm (patches 1-9)
> Part 2: fixes for creating id 0 subflow (patches 10-13)
> Part 3: v4-mapped addr support (patches 14-15)
> Part 4: flush for userspace pm (patches 16-18)
> Part 5: address entry refcount for userspace pm (patches 19-25)

For tracking purposes, which patches can we drop on Patchwork?

https://patchwork.kernel.org/project/mptcp/list/?series=808195&state=7

- Part 1: If I understood correctly, patches 1-9 are supposed to be
replaced by a new series "dump for userspace pm", right? Then can we
drop patches 1, 2 and 5 from Patchwork? (Patch 3 has been marked as
"Rejected", I guess that's OK.)

- Part 2: does it depend on part 1, or can it be applied separatelly in
-net because they are fixes? Or at least the modifications in the code,
the selftests tests could go only in -next if it is difficult to get
them in -net.

- Part 3: Is it also a fix? I mean: did we forget to support v4-mapped
addr in one command, but others have this support? Can it be sent
separately to -net?

- Part 4: it is not clear to me if they were needed? They have been
marked as "Rejected", but please tell me if they are still needed and
just the title here was wrong.

- Part 5: I need to re-check the code before invalidating them (and
issue #403).

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Hi Matt,

On Wed, 2023-12-27 at 12:38 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/12/2023 11:07, Geliang Tang wrote:
> > v14:
> >  - implement flush operation in user space as Mat suggested.
> >  - update selftests.
> >  - Now this series includes five parts:
> > 
> > Part 1: dump for userspace pm (patches 1-9)
> > Part 2: fixes for creating id 0 subflow (patches 10-13)
> > Part 3: v4-mapped addr support (patches 14-15)
> > Part 4: flush for userspace pm (patches 16-18)
> > Part 5: address entry refcount for userspace pm (patches 19-25)
> 
> For tracking purposes, which patches can we drop on Patchwork?
> 
> https://patchwork.kernel.org/project/mptcp/list/?series=808195&state=7
> 
> - Part 1: If I understood correctly, patches 1-9 are supposed to be
> replaced by a new series "dump for userspace pm", right? Then can we
> drop patches 1, 2 and 5 from Patchwork? (Patch 3 has been marked as
> "Rejected", I guess that's OK.)

Patches 1, 2 and 3 can been marked as "Rejected".
Patches 4-9 can been marked as "Changes Requested", they will
been included in the next version.

> 
> - Part 2: does it depend on part 1, or can it be applied separatelly
> in
> -net because they are fixes? Or at least the modifications in the
> code,
> the selftests tests could go only in -next if it is difficult to get
> them in -net.
> 
> - Part 3: Is it also a fix? I mean: did we forget to support v4-
> mapped
> addr in one command, but others have this support? Can it be sent
> separately to -net?

Yes, patches 10, 11, 12, 14 are fixes. They don't depend on part 1 but
will conflict with part 1. I'll make them a new series for -net.

> 
> - Part 4: it is not clear to me if they were needed? They have been
> marked as "Rejected", but please tell me if they are still needed and
> just the title here was wrong.

Patch 18 can be marked as "Changes Requested". We can test flushing
addresses in userspace in it.

Thanks,
-Geliang

> 
> - Part 5: I need to re-check the code before invalidating them (and
> issue #403).
> 
> Cheers,
> Matt

Hi Geliang,

Thank you for your reply!

On 28/12/2023 03:31, Geliang Tang wrote:
> Hi Matt,
> 
> On Wed, 2023-12-27 at 12:38 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 08/12/2023 11:07, Geliang Tang wrote:
>>> v14:
>>>  - implement flush operation in user space as Mat suggested.
>>>  - update selftests.
>>>  - Now this series includes five parts:
>>>
>>> Part 1: dump for userspace pm (patches 1-9)
>>> Part 2: fixes for creating id 0 subflow (patches 10-13)
>>> Part 3: v4-mapped addr support (patches 14-15)
>>> Part 4: flush for userspace pm (patches 16-18)
>>> Part 5: address entry refcount for userspace pm (patches 19-25)
>>
>> For tracking purposes, which patches can we drop on Patchwork?
>>
>> https://patchwork.kernel.org/project/mptcp/list/?series=808195&state=7
>>
>> - Part 1: If I understood correctly, patches 1-9 are supposed to be
>> replaced by a new series "dump for userspace pm", right? Then can we
>> drop patches 1, 2 and 5 from Patchwork? (Patch 3 has been marked as
>> "Rejected", I guess that's OK.)
> 
> Patches 1, 2 and 3 can been marked as "Rejected".

Done!

> Patches 4-9 can been marked as "Changes Requested", they will
> been included in the next version.

I marked these patches (except patch 5) as "Superseded" because there is
already a new version if I'm not mistaken: now part of "dump for
userspace pm" series, right?

Or maybe patch 5 has also been replaced by another patch?

>> - Part 2: does it depend on part 1, or can it be applied separatelly
>> in
>> -net because they are fixes? Or at least the modifications in the
>> code,
>> the selftests tests could go only in -next if it is difficult to get
>> them in -net.
>>
>> - Part 3: Is it also a fix? I mean: did we forget to support v4-
>> mapped
>> addr in one command, but others have this support? Can it be sent
>> separately to -net?
> 
> Yes, patches 10, 11, 12, 14 are fixes. They don't depend on part 1 but
> will conflict with part 1. I'll make them a new series for -net.

Thank you, so we can send them to -net and have them backported in
stable versions.

>> - Part 4: it is not clear to me if they were needed? They have been
>> marked as "Rejected", but please tell me if they are still needed and
>> just the title here was wrong.
> 
> Patch 18 can be marked as "Changes Requested". We can test flushing
> addresses in userspace in it.

We can indeed check that it doesn't affect addresses from the userspace
PM. I just marked it as "Changes Requested".

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Hi Matt,

On Thu, 2023-12-28 at 10:44 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your reply!
> 
> On 28/12/2023 03:31, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Wed, 2023-12-27 at 12:38 +0100, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 08/12/2023 11:07, Geliang Tang wrote:
> > > > v14:
> > > >  - implement flush operation in user space as Mat suggested.
> > > >  - update selftests.
> > > >  - Now this series includes five parts:
> > > > 
> > > > Part 1: dump for userspace pm (patches 1-9)
> > > > Part 2: fixes for creating id 0 subflow (patches 10-13)
> > > > Part 3: v4-mapped addr support (patches 14-15)
> > > > Part 4: flush for userspace pm (patches 16-18)
> > > > Part 5: address entry refcount for userspace pm (patches 19-25)
> > > 
> > > For tracking purposes, which patches can we drop on Patchwork?
> > > 
> > > https://patchwork.kernel.org/project/mptcp/list/?series=808195&state=7
> > > 
> > > - Part 1: If I understood correctly, patches 1-9 are supposed to
> > > be
> > > replaced by a new series "dump for userspace pm", right? Then can
> > > we
> > > drop patches 1, 2 and 5 from Patchwork? (Patch 3 has been marked
> > > as
> > > "Rejected", I guess that's OK.)
> > 
> > Patches 1, 2 and 3 can been marked as "Rejected".
> 
> Done!
> 
> > Patches 4-9 can been marked as "Changes Requested", they will
> > been included in the next version.
> 
> I marked these patches (except patch 5) as "Superseded" because there
> is
> already a new version if I'm not mistaken: now part of "dump for
> userspace pm" series, right?
> 
> Or maybe patch 5 has also been replaced by another patch?

Yes, you're right. Patch 5 will be resent as a cleanup patch.

Thanks,
-Geliang

> 
> > > - Part 2: does it depend on part 1, or can it be applied
> > > separatelly
> > > in
> > > -net because they are fixes? Or at least the modifications in the
> > > code,
> > > the selftests tests could go only in -next if it is difficult to
> > > get
> > > them in -net.
> > > 
> > > - Part 3: Is it also a fix? I mean: did we forget to support v4-
> > > mapped
> > > addr in one command, but others have this support? Can it be sent
> > > separately to -net?
> > 
> > Yes, patches 10, 11, 12, 14 are fixes. They don't depend on part 1
> > but
> > will conflict with part 1. I'll make them a new series for -net.
> 
> Thank you, so we can send them to -net and have them backported in
> stable versions.
> 
> > > - Part 4: it is not clear to me if they were needed? They have
> > > been
> > > marked as "Rejected", but please tell me if they are still needed
> > > and
> > > just the title here was wrong.
> > 
> > Patch 18 can be marked as "Changes Requested". We can test flushing
> > addresses in userspace in it.
> 
> We can indeed check that it doesn't affect addresses from the
> userspace
> PM. I just marked it as "Changes Requested".
> 
> Cheers,
> Matt