The id_avail_bitmap is only used when either the 'subflow' or 'signal'
flag is used, but not with 'fullmesh' only. Here, it is replacing the
'subflow' action, so check if this flag is set.
Also, add the check for the max subflows upfront, so we avoid scheduling
the worker with MPTCP_PM_ADD_ADDR_RECEIVED if it is not needed.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2: modify protocol.h here too (Geliang)
---
net/mptcp/pm_kernel.c | 3 ++-
net/mptcp/protocol.h | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d7cd89fa6a11a1ea7703edbfbdf2bbe86a6a3054..277f81f38134d07918143331746a50bc316d81ca 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -411,7 +411,8 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
locals[i].flags = entry->flags;
locals[i].ifindex = entry->ifindex;
- if (c_flag_case)
+ if (c_flag_case &&
+ (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
__clear_bit(locals[i].addr.id,
msk->pm.id_avail_bitmap);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index dd0662defd41c84474e44c559c571e3594b85d9e..0d6dae37c9daf4ec8990b9a87036aa393add585c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1200,7 +1200,8 @@ static inline bool mptcp_pm_add_addr_c_flag_case(struct mptcp_sock *msk)
{
return READ_ONCE(msk->pm.remote_deny_join_id0) &&
msk->pm.local_addr_used == 0 &&
- mptcp_pm_get_add_addr_accept_max(msk) == 0;
+ mptcp_pm_get_add_addr_accept_max(msk) == 0 &&
+ msk->pm.subflows < mptcp_pm_get_subflows_max(msk);
}
void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk);
--
2.51.0Hi Matt,
Thanks for this set. It looks good to me overall. Only one comment
below.
On Fri, 2025-09-19 at 11:08 +0200, Matthieu Baerts (NGI0) wrote:
> The id_avail_bitmap is only used when either the 'subflow' or
> 'signal'
> flag is used, but not with 'fullmesh' only. Here, it is replacing the
> 'subflow' action, so check if this flag is set.
>
> Also, add the check for the max subflows upfront, so we avoid
> scheduling
> the worker with MPTCP_PM_ADD_ADDR_RECEIVED if it is not needed.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> v2: modify protocol.h here too (Geliang)
> ---
> net/mptcp/pm_kernel.c | 3 ++-
> net/mptcp/protocol.h | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index
> d7cd89fa6a11a1ea7703edbfbdf2bbe86a6a3054..277f81f38134d07918143331746
> a50bc316d81ca 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -411,7 +411,8 @@ static unsigned int
> fill_local_addresses_vec(struct mptcp_sock *msk,
> locals[i].flags = entry->flags;
> locals[i].ifindex = entry->ifindex;
>
> - if (c_flag_case)
> + if (c_flag_case &&
> + (entry->flags &
> MPTCP_PM_ADDR_FLAG_SUBFLOW))
> __clear_bit(locals[i].addr.id,
> msk-
> >pm.id_avail_bitmap);
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index
> dd0662defd41c84474e44c559c571e3594b85d9e..0d6dae37c9daf4ec8990b9a8703
> 6aa393add585c 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1200,7 +1200,8 @@ static inline bool
> mptcp_pm_add_addr_c_flag_case(struct mptcp_sock *msk)
> {
> return READ_ONCE(msk->pm.remote_deny_join_id0) &&
> msk->pm.local_addr_used == 0 &&
> - mptcp_pm_get_add_addr_accept_max(msk) == 0;
> + mptcp_pm_get_add_addr_accept_max(msk) == 0 &&
> + msk->pm.subflows < mptcp_pm_get_subflows_max(msk);
If this line is added here, no need to change it again in
fill_local_addresses_vec() in "mptcp: pm: in-kernel: usable client side
with C-flag", right? I mean we can drop "msk->pm.subflows <
subflows_max" in this while statement:
while (msk->pm.local_addr_used < local_addr_max &&
msk->pm.subflows < subflows_max) {
No need to send a v3, please update it when merging it if you agree.
> }
>
> void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock
> *ssk);
Hi Geliang,
On 19/09/2025 12:11, Geliang Tang wrote:
> Hi Matt,
>
> Thanks for this set. It looks good to me overall. Only one comment
> below.
>
> On Fri, 2025-09-19 at 11:08 +0200, Matthieu Baerts (NGI0) wrote:
>> The id_avail_bitmap is only used when either the 'subflow' or
>> 'signal'
>> flag is used, but not with 'fullmesh' only. Here, it is replacing the
>> 'subflow' action, so check if this flag is set.
>>
>> Also, add the check for the max subflows upfront, so we avoid
>> scheduling
>> the worker with MPTCP_PM_ADD_ADDR_RECEIVED if it is not needed.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> v2: modify protocol.h here too (Geliang)
>> ---
>> net/mptcp/pm_kernel.c | 3 ++-
>> net/mptcp/protocol.h | 3 ++-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>> index
>> d7cd89fa6a11a1ea7703edbfbdf2bbe86a6a3054..277f81f38134d07918143331746
>> a50bc316d81ca 100644
>> --- a/net/mptcp/pm_kernel.c
>> +++ b/net/mptcp/pm_kernel.c
>> @@ -411,7 +411,8 @@ static unsigned int
>> fill_local_addresses_vec(struct mptcp_sock *msk,
>> locals[i].flags = entry->flags;
>> locals[i].ifindex = entry->ifindex;
>>
>> - if (c_flag_case)
>> + if (c_flag_case &&
>> + (entry->flags &
>> MPTCP_PM_ADDR_FLAG_SUBFLOW))
>> __clear_bit(locals[i].addr.id,
>> msk-
>>> pm.id_avail_bitmap);
>>
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index
>> dd0662defd41c84474e44c559c571e3594b85d9e..0d6dae37c9daf4ec8990b9a8703
>> 6aa393add585c 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -1200,7 +1200,8 @@ static inline bool
>> mptcp_pm_add_addr_c_flag_case(struct mptcp_sock *msk)
>> {
>> return READ_ONCE(msk->pm.remote_deny_join_id0) &&
>> msk->pm.local_addr_used == 0 &&
>> - mptcp_pm_get_add_addr_accept_max(msk) == 0;
>> + mptcp_pm_get_add_addr_accept_max(msk) == 0 &&
>> + msk->pm.subflows < mptcp_pm_get_subflows_max(msk);
>
> If this line is added here, no need to change it again in
> fill_local_addresses_vec() in "mptcp: pm: in-kernel: usable client side
> with C-flag", right? I mean we can drop "msk->pm.subflows <
> subflows_max" in this while statement:
>
> while (msk->pm.local_addr_used < local_addr_max &&
> msk->pm.subflows < subflows_max) {
>
> No need to send a v3, please update it when merging it if you agree.
No, we still need it here, because we might create more than one subflow.
We could move it to the end of the loop, but I would rather prefer not
to do that in the patch for 'net'. Probably better to keep this patch
simple, and similar to what is done in fill_local_addresses_vec() for
the fullmesh case.
In the refactoring, we can apply the same modifications for both the
fullmesh case and the one here: using a break at the end of the loop. WDYT?
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
On Fri, 2025-09-19 at 12:30 +0200, Matthieu Baerts wrote:
> Hi Geliang,
>
> On 19/09/2025 12:11, Geliang Tang wrote:
> > Hi Matt,
> >
> > Thanks for this set. It looks good to me overall. Only one comment
> > below.
> >
> > On Fri, 2025-09-19 at 11:08 +0200, Matthieu Baerts (NGI0) wrote:
> > > The id_avail_bitmap is only used when either the 'subflow' or
> > > 'signal'
> > > flag is used, but not with 'fullmesh' only. Here, it is replacing
> > > the
> > > 'subflow' action, so check if this flag is set.
> > >
> > > Also, add the check for the max subflows upfront, so we avoid
> > > scheduling
> > > the worker with MPTCP_PM_ADD_ADDR_RECEIVED if it is not needed.
> > >
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > v2: modify protocol.h here too (Geliang)
> > > ---
> > > net/mptcp/pm_kernel.c | 3 ++-
> > > net/mptcp/protocol.h | 3 ++-
> > > 2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> > > index
> > > d7cd89fa6a11a1ea7703edbfbdf2bbe86a6a3054..277f81f38134d0791814333
> > > 1746
> > > a50bc316d81ca 100644
> > > --- a/net/mptcp/pm_kernel.c
> > > +++ b/net/mptcp/pm_kernel.c
> > > @@ -411,7 +411,8 @@ static unsigned int
> > > fill_local_addresses_vec(struct mptcp_sock *msk,
> > > locals[i].flags = entry->flags;
> > > locals[i].ifindex = entry->ifindex;
> > >
> > > - if (c_flag_case)
> > > + if (c_flag_case &&
> > > + (entry->flags &
> > > MPTCP_PM_ADDR_FLAG_SUBFLOW))
> > > __clear_bit(locals[i].addr.id,
> > > msk-
> > > > pm.id_avail_bitmap);
> > >
> > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > > index
> > > dd0662defd41c84474e44c559c571e3594b85d9e..0d6dae37c9daf4ec8990b9a
> > > 8703
> > > 6aa393add585c 100644
> > > --- a/net/mptcp/protocol.h
> > > +++ b/net/mptcp/protocol.h
> > > @@ -1200,7 +1200,8 @@ static inline bool
> > > mptcp_pm_add_addr_c_flag_case(struct mptcp_sock *msk)
> > > {
> > > return READ_ONCE(msk->pm.remote_deny_join_id0) &&
> > > msk->pm.local_addr_used == 0 &&
> > > - mptcp_pm_get_add_addr_accept_max(msk) == 0;
> > > + mptcp_pm_get_add_addr_accept_max(msk) == 0 &&
> > > + msk->pm.subflows <
> > > mptcp_pm_get_subflows_max(msk);
> >
> > If this line is added here, no need to change it again in
> > fill_local_addresses_vec() in "mptcp: pm: in-kernel: usable client
> > side
> > with C-flag", right? I mean we can drop "msk->pm.subflows <
> > subflows_max" in this while statement:
> >
> > while (msk->pm.local_addr_used < local_addr_max &&
> > msk->pm.subflows < subflows_max) {
> >
> > No need to send a v3, please update it when merging it if you
> > agree.
>
> No, we still need it here, because we might create more than one
> subflow.
>
> We could move it to the end of the loop, but I would rather prefer
> not
> to do that in the patch for 'net'. Probably better to keep this patch
> simple, and similar to what is done in fill_local_addresses_vec() for
> the fullmesh case.
>
> In the refactoring, we can apply the same modifications for both the
> fullmesh case and the one here: using a break at the end of the loop.
> WDYT?
Sure, let's keep it as is. I just replied my tag in the cover letter.
Thanks,
-Geliang
>
> Cheers,
> Matt
© 2016 - 2025 Red Hat, Inc.