1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next 0/4] mptcp: pm: netlink: announce server-side flag
  4. View patch

[PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true

Matthieu Baerts (NGI0) posted 4 patches 1 week ago
[PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Matthieu Baerts (NGI0) 1 week ago 
This attribute is a boolean. No need to add it to set it to 'false'.

Indeed, the default value when this attribute is not set is naturally
'false'. A few bytes can then be saved by not adding this attribute if
the connection is not on the server side.

This prepares the future deprecation of its attribute, in favour of a
new flag.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 Documentation/netlink/specs/mptcp_pm.yaml         | 4 ++--
 include/uapi/linux/mptcp_pm.h                     | 4 ++--
 net/mptcp/pm_netlink.c                            | 4 +++-
 tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/netlink/specs/mptcp_pm.yaml b/Documentation/netlink/specs/mptcp_pm.yaml
index d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c0304efd3215cc24485ea22e1ede 100644
--- a/Documentation/netlink/specs/mptcp_pm.yaml
+++ b/Documentation/netlink/specs/mptcp_pm.yaml
@@ -28,13 +28,13 @@ definitions:
           traffic-patterns it can take a long time until the
           MPTCP_EVENT_ESTABLISHED is sent.
           Attributes: token, family, saddr4 | saddr6, daddr4 | daddr6, sport,
-          dport, server-side, [flags].
+          dport, [server-side], [flags].
       -
         name: established
         doc: >-
           A MPTCP connection is established (can start new subflows).
           Attributes: token, family, saddr4 | saddr6, daddr4 | daddr6, sport,
-          dport, server-side, [flags].
+          dport, [server-side], [flags].
       -
         name: closed
         doc: >-
diff --git a/include/uapi/linux/mptcp_pm.h b/include/uapi/linux/mptcp_pm.h
index 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6d789632682a9bedbf8090feb9 100644
--- a/include/uapi/linux/mptcp_pm.h
+++ b/include/uapi/linux/mptcp_pm.h
@@ -16,10 +16,10 @@
  *   good time to allocate memory and send ADD_ADDR if needed. Depending on the
  *   traffic-patterns it can take a long time until the MPTCP_EVENT_ESTABLISHED
  *   is sent. Attributes: token, family, saddr4 | saddr6, daddr4 | daddr6,
- *   sport, dport, server-side, [flags].
+ *   sport, dport, [server-side], [flags].
  * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is established (can start new
  *   subflows). Attributes: token, family, saddr4 | saddr6, daddr4 | daddr6,
- *   sport, dport, server-side, [flags].
+ *   sport, dport, [server-side], [flags].
  * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped. Attribute: token.
  * @MPTCP_EVENT_ANNOUNCED: A new address has been announced by the peer.
  *   Attributes: token, rem_id, family, daddr4 | daddr6 [, dport].
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b59717472aca2d38add26255419 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -413,7 +413,9 @@ static int mptcp_event_created(struct sk_buff *skb,
 	if (err)
 		return err;
 
-	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, READ_ONCE(msk->pm.server_side)))
+	/* only set when it is the server side */
+	if (READ_ONCE(msk->pm.server_side) &&
+	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
 		return -EMSGSIZE;
 
 	if (READ_ONCE(msk->pm.remote_deny_join_id0))
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 0801d57a9710406a5942af742719f694e3907558..189bfc5bf86611575381542fe59b86660f335294 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -241,7 +241,7 @@ make_connection()
 
 	print_test "Established IP${is_v6} MPTCP Connection ns2 => ns1"
 	if [ "${client_token}" != "" ] && [ "${server_token}" != "" ] &&
-	   [ "${client_serverside}" = 0 ] && [ "${server_serverside}" = 1 ] &&
+	   [ "${client_serverside:-0}" = 0 ] && [ "${server_serverside:-0}" = 1 ] &&
 	   [ "${client_nojoin:-0}" = 1 ] && [ "${server_nojoin:-0}" = 0 ]
 	then
 		test_pass

-- 
2.51.0
Re: [PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Geliang Tang 6 days, 8 hours ago 
Hi Matt,

On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0) wrote:
> This attribute is a boolean. No need to add it to set it to 'false'.
> 
> Indeed, the default value when this attribute is not set is naturally
> 'false'. A few bytes can then be saved by not adding this attribute
> if
> the connection is not on the server side.
> 
> This prepares the future deprecation of its attribute, in favour of a
> new flag.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  Documentation/netlink/specs/mptcp_pm.yaml         | 4 ++--
>  include/uapi/linux/mptcp_pm.h                     | 4 ++--
>  net/mptcp/pm_netlink.c                            | 4 +++-
>  tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
>  4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/mptcp_pm.yaml
> b/Documentation/netlink/specs/mptcp_pm.yaml
> index
> d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c0304efd3215cc2
> 4485ea22e1ede 100644
> --- a/Documentation/netlink/specs/mptcp_pm.yaml
> +++ b/Documentation/netlink/specs/mptcp_pm.yaml
> @@ -28,13 +28,13 @@ definitions:
>            traffic-patterns it can take a long time until the
>            MPTCP_EVENT_ESTABLISHED is sent.
>            Attributes: token, family, saddr4 | saddr6, daddr4 |
> daddr6, sport,
> -          dport, server-side, [flags].
> +          dport, [server-side], [flags].
>        -
>          name: established
>          doc: >-
>            A MPTCP connection is established (can start new
> subflows).
>            Attributes: token, family, saddr4 | saddr6, daddr4 |
> daddr6, sport,
> -          dport, server-side, [flags].
> +          dport, [server-side], [flags].
>        -
>          name: closed
>          doc: >-
> diff --git a/include/uapi/linux/mptcp_pm.h
> b/include/uapi/linux/mptcp_pm.h
> index
> 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6d789632682a9
> bedbf8090feb9 100644
> --- a/include/uapi/linux/mptcp_pm.h
> +++ b/include/uapi/linux/mptcp_pm.h
> @@ -16,10 +16,10 @@
>   *   good time to allocate memory and send ADD_ADDR if needed.
> Depending on the
>   *   traffic-patterns it can take a long time until the
> MPTCP_EVENT_ESTABLISHED
>   *   is sent. Attributes: token, family, saddr4 | saddr6, daddr4 |
> daddr6,
> - *   sport, dport, server-side, [flags].
> + *   sport, dport, [server-side], [flags].
>   * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is established (can
> start new
>   *   subflows). Attributes: token, family, saddr4 | saddr6, daddr4 |
> daddr6,
> - *   sport, dport, server-side, [flags].
> + *   sport, dport, [server-side], [flags].
>   * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped. Attribute:
> token.
>   * @MPTCP_EVENT_ANNOUNCED: A new address has been announced by the
> peer.
>   *   Attributes: token, rem_id, family, daddr4 | daddr6 [, dport].
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index
> 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b59717472aca2d
> 38add26255419 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -413,7 +413,9 @@ static int mptcp_event_created(struct sk_buff
> *skb,
>  	if (err)
>  		return err;
>  
> -	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, READ_ONCE(msk-
> >pm.server_side)))
> +	/* only set when it is the server side */
> +	if (READ_ONCE(msk->pm.server_side) &&
> +	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))

In patch 2, this will be modified to use two 'if':

	if (READ_ONCE(msk->pm.server_side)) {
		flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;

		/* only set when it is the server side */
		if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
			return -EMSGSIZE;
	}

Why don't we just use two 'if' statements here, which will make patch 2
simpler:

       if (READ_ONCE(msk->pm.server_side)) {
               /* only set when it is the server side */
               if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
                       return -EMSGSIZE;
       }

WDYT?

Thanks,
-Geliang

>  		return -EMSGSIZE;
>  
>  	if (READ_ONCE(msk->pm.remote_deny_join_id0))
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index
> 0801d57a9710406a5942af742719f694e3907558..189bfc5bf86611575381542fe59
> b86660f335294 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -241,7 +241,7 @@ make_connection()
>  
>  	print_test "Established IP${is_v6} MPTCP Connection ns2 =>
> ns1"
>  	if [ "${client_token}" != "" ] && [ "${server_token}" != ""
> ] &&
> -	   [ "${client_serverside}" = 0 ] && [
> "${server_serverside}" = 1 ] &&
> +	   [ "${client_serverside:-0}" = 0 ] && [
> "${server_serverside:-0}" = 1 ] &&
>  	   [ "${client_nojoin:-0}" = 1 ] && [ "${server_nojoin:-0}"
> = 0 ]
>  	then
>  		test_pass
Re: [PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Matthieu Baerts 6 days, 7 hours ago 
Hi Geliang,

Thank you for the review!

On 11/09/2025 10:15, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0) wrote:
>> This attribute is a boolean. No need to add it to set it to 'false'.
>>
>> Indeed, the default value when this attribute is not set is naturally
>> 'false'. A few bytes can then be saved by not adding this attribute
>> if
>> the connection is not on the server side.
>>
>> This prepares the future deprecation of its attribute, in favour of a
>> new flag.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  Documentation/netlink/specs/mptcp_pm.yaml         | 4 ++--
>>  include/uapi/linux/mptcp_pm.h                     | 4 ++--
>>  net/mptcp/pm_netlink.c                            | 4 +++-
>>  tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
>>  4 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/netlink/specs/mptcp_pm.yaml
>> b/Documentation/netlink/specs/mptcp_pm.yaml
>> index
>> d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c0304efd3215cc2
>> 4485ea22e1ede 100644
>> --- a/Documentation/netlink/specs/mptcp_pm.yaml
>> +++ b/Documentation/netlink/specs/mptcp_pm.yaml
>> @@ -28,13 +28,13 @@ definitions:
>>            traffic-patterns it can take a long time until the
>>            MPTCP_EVENT_ESTABLISHED is sent.
>>            Attributes: token, family, saddr4 | saddr6, daddr4 |
>> daddr6, sport,
>> -          dport, server-side, [flags].
>> +          dport, [server-side], [flags].
>>        -
>>          name: established
>>          doc: >-
>>            A MPTCP connection is established (can start new
>> subflows).
>>            Attributes: token, family, saddr4 | saddr6, daddr4 |
>> daddr6, sport,
>> -          dport, server-side, [flags].
>> +          dport, [server-side], [flags].
>>        -
>>          name: closed
>>          doc: >-
>> diff --git a/include/uapi/linux/mptcp_pm.h
>> b/include/uapi/linux/mptcp_pm.h
>> index
>> 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6d789632682a9
>> bedbf8090feb9 100644
>> --- a/include/uapi/linux/mptcp_pm.h
>> +++ b/include/uapi/linux/mptcp_pm.h
>> @@ -16,10 +16,10 @@
>>   *   good time to allocate memory and send ADD_ADDR if needed.
>> Depending on the
>>   *   traffic-patterns it can take a long time until the
>> MPTCP_EVENT_ESTABLISHED
>>   *   is sent. Attributes: token, family, saddr4 | saddr6, daddr4 |
>> daddr6,
>> - *   sport, dport, server-side, [flags].
>> + *   sport, dport, [server-side], [flags].
>>   * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is established (can
>> start new
>>   *   subflows). Attributes: token, family, saddr4 | saddr6, daddr4 |
>> daddr6,
>> - *   sport, dport, server-side, [flags].
>> + *   sport, dport, [server-side], [flags].
>>   * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped. Attribute:
>> token.
>>   * @MPTCP_EVENT_ANNOUNCED: A new address has been announced by the
>> peer.
>>   *   Attributes: token, rem_id, family, daddr4 | daddr6 [, dport].
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index
>> 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b59717472aca2d
>> 38add26255419 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -413,7 +413,9 @@ static int mptcp_event_created(struct sk_buff
>> *skb,
>>  	if (err)
>>  		return err;
>>  
>> -	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, READ_ONCE(msk-
>>> pm.server_side)))
>> +	/* only set when it is the server side */
>> +	if (READ_ONCE(msk->pm.server_side) &&
>> +	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> 
> In patch 2, this will be modified to use two 'if':
> 
> 	if (READ_ONCE(msk->pm.server_side)) {
> 		flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
> 
> 		/* only set when it is the server side */
> 		if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> 			return -EMSGSIZE;
> 	}
> 
> Why don't we just use two 'if' statements here, which will make patch 2
> simpler:
> 
>        if (READ_ONCE(msk->pm.server_side)) {
>                /* only set when it is the server side */
>                if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
>                        return -EMSGSIZE;
>        }
> 
> WDYT?

I'm not convinced: that might make the patch 2 simpler, but then the
patch 1 will raise questions from reviewers and developers looking at
the patches in the order they have been sent: why using 2 'if'
statements while there is no need to use 2.

If we want that, I would have to add something in the commit message. I
don't think that's worth it.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Geliang Tang 6 days, 7 hours ago 
Hi Matt,

On Thu, 2025-09-11 at 10:20 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 11/09/2025 10:15, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0) wrote:
> > > This attribute is a boolean. No need to add it to set it to
> > > 'false'.
> > > 
> > > Indeed, the default value when this attribute is not set is
> > > naturally
> > > 'false'. A few bytes can then be saved by not adding this
> > > attribute
> > > if
> > > the connection is not on the server side.
> > > 
> > > This prepares the future deprecation of its attribute, in favour
> > > of a
> > > new flag.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  Documentation/netlink/specs/mptcp_pm.yaml         | 4 ++--
> > >  include/uapi/linux/mptcp_pm.h                     | 4 ++--
> > >  net/mptcp/pm_netlink.c                            | 4 +++-
> > >  tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
> > >  4 files changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/Documentation/netlink/specs/mptcp_pm.yaml
> > > b/Documentation/netlink/specs/mptcp_pm.yaml
> > > index
> > > d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c0304efd321
> > > 5cc2
> > > 4485ea22e1ede 100644
> > > --- a/Documentation/netlink/specs/mptcp_pm.yaml
> > > +++ b/Documentation/netlink/specs/mptcp_pm.yaml
> > > @@ -28,13 +28,13 @@ definitions:
> > >            traffic-patterns it can take a long time until the
> > >            MPTCP_EVENT_ESTABLISHED is sent.
> > >            Attributes: token, family, saddr4 | saddr6, daddr4 |
> > > daddr6, sport,
> > > -          dport, server-side, [flags].
> > > +          dport, [server-side], [flags].
> > >        -
> > >          name: established
> > >          doc: >-
> > >            A MPTCP connection is established (can start new
> > > subflows).
> > >            Attributes: token, family, saddr4 | saddr6, daddr4 |
> > > daddr6, sport,
> > > -          dport, server-side, [flags].
> > > +          dport, [server-side], [flags].
> > >        -
> > >          name: closed
> > >          doc: >-
> > > diff --git a/include/uapi/linux/mptcp_pm.h
> > > b/include/uapi/linux/mptcp_pm.h
> > > index
> > > 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6d7896326
> > > 82a9
> > > bedbf8090feb9 100644
> > > --- a/include/uapi/linux/mptcp_pm.h
> > > +++ b/include/uapi/linux/mptcp_pm.h
> > > @@ -16,10 +16,10 @@
> > >   *   good time to allocate memory and send ADD_ADDR if needed.
> > > Depending on the
> > >   *   traffic-patterns it can take a long time until the
> > > MPTCP_EVENT_ESTABLISHED
> > >   *   is sent. Attributes: token, family, saddr4 | saddr6, daddr4
> > > |
> > > daddr6,
> > > - *   sport, dport, server-side, [flags].
> > > + *   sport, dport, [server-side], [flags].
> > >   * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is established
> > > (can
> > > start new
> > >   *   subflows). Attributes: token, family, saddr4 | saddr6,
> > > daddr4 |
> > > daddr6,
> > > - *   sport, dport, server-side, [flags].
> > > + *   sport, dport, [server-side], [flags].
> > >   * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped.
> > > Attribute:
> > > token.
> > >   * @MPTCP_EVENT_ANNOUNCED: A new address has been announced by
> > > the
> > > peer.
> > >   *   Attributes: token, rem_id, family, daddr4 | daddr6 [,
> > > dport].
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index
> > > 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b59717472a
> > > ca2d
> > > 38add26255419 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -413,7 +413,9 @@ static int mptcp_event_created(struct sk_buff
> > > *skb,
> > >  	if (err)
> > >  		return err;
> > >  
> > > -	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
> > > READ_ONCE(msk-
> > > > pm.server_side)))
> > > +	/* only set when it is the server side */
> > > +	if (READ_ONCE(msk->pm.server_side) &&
> > > +	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> > 
> > In patch 2, this will be modified to use two 'if':
> > 
> > 	if (READ_ONCE(msk->pm.server_side)) {
> > 		flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
> > 
> > 		/* only set when it is the server side */
> > 		if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> > 			return -EMSGSIZE;
> > 	}
> > 
> > Why don't we just use two 'if' statements here, which will make
> > patch 2
> > simpler:
> > 
> >        if (READ_ONCE(msk->pm.server_side)) {
> >                /* only set when it is the server side */
> >                if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> >                        return -EMSGSIZE;
> >        }
> > 
> > WDYT?
> 
> I'm not convinced: that might make the patch 2 simpler, but then the
> patch 1 will raise questions from reviewers and developers looking at
> the patches in the order they have been sent: why using 2 'if'
> statements while there is no need to use 2.
> 
> If we want that, I would have to add something in the commit message.
> I
> don't think that's worth it.

Sure. Let's keep this patch as is. If so, could you please update the
position of the comments in patch 2 and patch 3? Something like:

        /* only set when it is the server side */
        if (READ_ONCE(msk->pm.server_side)) {
                flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;

                /* Deprecated */
                if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1)) 
                        return -EMSGSIZE;
        }

Thanks,
-Geliang

> 
> Cheers,
> Matt
Re: [PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Matthieu Baerts 6 days, 7 hours ago 
Hi Geliang,

On 11/09/2025 10:33, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, 2025-09-11 at 10:20 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the review!
>>
>> On 11/09/2025 10:15, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0) wrote:
>>>> This attribute is a boolean. No need to add it to set it to
>>>> 'false'.
>>>>
>>>> Indeed, the default value when this attribute is not set is
>>>> naturally
>>>> 'false'. A few bytes can then be saved by not adding this
>>>> attribute
>>>> if
>>>> the connection is not on the server side.
>>>>
>>>> This prepares the future deprecation of its attribute, in favour
>>>> of a
>>>> new flag.
>>>>
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  Documentation/netlink/specs/mptcp_pm.yaml         | 4 ++--
>>>>  include/uapi/linux/mptcp_pm.h                     | 4 ++--
>>>>  net/mptcp/pm_netlink.c                            | 4 +++-
>>>>  tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
>>>>  4 files changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/netlink/specs/mptcp_pm.yaml
>>>> b/Documentation/netlink/specs/mptcp_pm.yaml
>>>> index
>>>> d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c0304efd321
>>>> 5cc2
>>>> 4485ea22e1ede 100644
>>>> --- a/Documentation/netlink/specs/mptcp_pm.yaml
>>>> +++ b/Documentation/netlink/specs/mptcp_pm.yaml
>>>> @@ -28,13 +28,13 @@ definitions:
>>>>            traffic-patterns it can take a long time until the
>>>>            MPTCP_EVENT_ESTABLISHED is sent.
>>>>            Attributes: token, family, saddr4 | saddr6, daddr4 |
>>>> daddr6, sport,
>>>> -          dport, server-side, [flags].
>>>> +          dport, [server-side], [flags].
>>>>        -
>>>>          name: established
>>>>          doc: >-
>>>>            A MPTCP connection is established (can start new
>>>> subflows).
>>>>            Attributes: token, family, saddr4 | saddr6, daddr4 |
>>>> daddr6, sport,
>>>> -          dport, server-side, [flags].
>>>> +          dport, [server-side], [flags].
>>>>        -
>>>>          name: closed
>>>>          doc: >-
>>>> diff --git a/include/uapi/linux/mptcp_pm.h
>>>> b/include/uapi/linux/mptcp_pm.h
>>>> index
>>>> 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6d7896326
>>>> 82a9
>>>> bedbf8090feb9 100644
>>>> --- a/include/uapi/linux/mptcp_pm.h
>>>> +++ b/include/uapi/linux/mptcp_pm.h
>>>> @@ -16,10 +16,10 @@
>>>>   *   good time to allocate memory and send ADD_ADDR if needed.
>>>> Depending on the
>>>>   *   traffic-patterns it can take a long time until the
>>>> MPTCP_EVENT_ESTABLISHED
>>>>   *   is sent. Attributes: token, family, saddr4 | saddr6, daddr4
>>>> |
>>>> daddr6,
>>>> - *   sport, dport, server-side, [flags].
>>>> + *   sport, dport, [server-side], [flags].
>>>>   * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is established
>>>> (can
>>>> start new
>>>>   *   subflows). Attributes: token, family, saddr4 | saddr6,
>>>> daddr4 |
>>>> daddr6,
>>>> - *   sport, dport, server-side, [flags].
>>>> + *   sport, dport, [server-side], [flags].
>>>>   * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped.
>>>> Attribute:
>>>> token.
>>>>   * @MPTCP_EVENT_ANNOUNCED: A new address has been announced by
>>>> the
>>>> peer.
>>>>   *   Attributes: token, rem_id, family, daddr4 | daddr6 [,
>>>> dport].
>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>> index
>>>> 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b59717472a
>>>> ca2d
>>>> 38add26255419 100644
>>>> --- a/net/mptcp/pm_netlink.c
>>>> +++ b/net/mptcp/pm_netlink.c
>>>> @@ -413,7 +413,9 @@ static int mptcp_event_created(struct sk_buff
>>>> *skb,
>>>>  	if (err)
>>>>  		return err;
>>>>  
>>>> -	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
>>>> READ_ONCE(msk-
>>>>> pm.server_side)))
>>>> +	/* only set when it is the server side */
>>>> +	if (READ_ONCE(msk->pm.server_side) &&
>>>> +	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
>>>
>>> In patch 2, this will be modified to use two 'if':
>>>
>>> 	if (READ_ONCE(msk->pm.server_side)) {
>>> 		flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
>>>
>>> 		/* only set when it is the server side */
>>> 		if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
>>> 			return -EMSGSIZE;
>>> 	}
>>>
>>> Why don't we just use two 'if' statements here, which will make
>>> patch 2
>>> simpler:
>>>
>>>        if (READ_ONCE(msk->pm.server_side)) {
>>>                /* only set when it is the server side */
>>>                if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
>>>                        return -EMSGSIZE;
>>>        }
>>>
>>> WDYT?
>>
>> I'm not convinced: that might make the patch 2 simpler, but then the
>> patch 1 will raise questions from reviewers and developers looking at
>> the patches in the order they have been sent: why using 2 'if'
>> statements while there is no need to use 2.
>>
>> If we want that, I would have to add something in the commit message.
>> I
>> don't think that's worth it.
> 
> Sure. Let's keep this patch as is. If so, could you please update the
> position of the comments in patch 2 and patch 3? Something like:
> 
>         /* only set when it is the server side */
>         if (READ_ONCE(msk->pm.server_side)) {
>                 flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;

Mmh, I'm not sure: for the flag, it makes sense to only set it when it
is the server side, otherwise that's wrong. Keeping the comment above is
then a bit confusing, no?

> 
>                 /* Deprecated */
>                 if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1)) 
>                         return -EMSGSIZE;
>         }
> 
> Thanks,
> -Geliang
> 
>>
>> Cheers,
>> Matt

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Geliang Tang 6 days, 7 hours ago 
On Thu, 2025-09-11 at 10:37 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 11/09/2025 10:33, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Thu, 2025-09-11 at 10:20 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > Thank you for the review!
> > > 
> > > On 11/09/2025 10:15, Geliang Tang wrote:
> > > > Hi Matt,
> > > > 
> > > > On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0)
> > > > wrote:
> > > > > This attribute is a boolean. No need to add it to set it to
> > > > > 'false'.
> > > > > 
> > > > > Indeed, the default value when this attribute is not set is
> > > > > naturally
> > > > > 'false'. A few bytes can then be saved by not adding this
> > > > > attribute
> > > > > if
> > > > > the connection is not on the server side.
> > > > > 
> > > > > This prepares the future deprecation of its attribute, in
> > > > > favour
> > > > > of a
> > > > > new flag.
> > > > > 
> > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > > > ---
> > > > >  Documentation/netlink/specs/mptcp_pm.yaml         | 4 ++--
> > > > >  include/uapi/linux/mptcp_pm.h                     | 4 ++--
> > > > >  net/mptcp/pm_netlink.c                            | 4 +++-
> > > > >  tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
> > > > >  4 files changed, 8 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > b/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > index
> > > > > d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c0304ef
> > > > > d321
> > > > > 5cc2
> > > > > 4485ea22e1ede 100644
> > > > > --- a/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > +++ b/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > @@ -28,13 +28,13 @@ definitions:
> > > > >            traffic-patterns it can take a long time until the
> > > > >            MPTCP_EVENT_ESTABLISHED is sent.
> > > > >            Attributes: token, family, saddr4 | saddr6, daddr4
> > > > > |
> > > > > daddr6, sport,
> > > > > -          dport, server-side, [flags].
> > > > > +          dport, [server-side], [flags].
> > > > >        -
> > > > >          name: established
> > > > >          doc: >-
> > > > >            A MPTCP connection is established (can start new
> > > > > subflows).
> > > > >            Attributes: token, family, saddr4 | saddr6, daddr4
> > > > > |
> > > > > daddr6, sport,
> > > > > -          dport, server-side, [flags].
> > > > > +          dport, [server-side], [flags].
> > > > >        -
> > > > >          name: closed
> > > > >          doc: >-
> > > > > diff --git a/include/uapi/linux/mptcp_pm.h
> > > > > b/include/uapi/linux/mptcp_pm.h
> > > > > index
> > > > > 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6d789
> > > > > 6326
> > > > > 82a9
> > > > > bedbf8090feb9 100644
> > > > > --- a/include/uapi/linux/mptcp_pm.h
> > > > > +++ b/include/uapi/linux/mptcp_pm.h
> > > > > @@ -16,10 +16,10 @@
> > > > >   *   good time to allocate memory and send ADD_ADDR if
> > > > > needed.
> > > > > Depending on the
> > > > >   *   traffic-patterns it can take a long time until the
> > > > > MPTCP_EVENT_ESTABLISHED
> > > > >   *   is sent. Attributes: token, family, saddr4 | saddr6,
> > > > > daddr4
> > > > > > 
> > > > > daddr6,
> > > > > - *   sport, dport, server-side, [flags].
> > > > > + *   sport, dport, [server-side], [flags].
> > > > >   * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is
> > > > > established
> > > > > (can
> > > > > start new
> > > > >   *   subflows). Attributes: token, family, saddr4 | saddr6,
> > > > > daddr4 |
> > > > > daddr6,
> > > > > - *   sport, dport, server-side, [flags].
> > > > > + *   sport, dport, [server-side], [flags].
> > > > >   * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped.
> > > > > Attribute:
> > > > > token.
> > > > >   * @MPTCP_EVENT_ANNOUNCED: A new address has been announced
> > > > > by
> > > > > the
> > > > > peer.
> > > > >   *   Attributes: token, rem_id, family, daddr4 | daddr6 [,
> > > > > dport].
> > > > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > > > index
> > > > > 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b59717
> > > > > 472a
> > > > > ca2d
> > > > > 38add26255419 100644
> > > > > --- a/net/mptcp/pm_netlink.c
> > > > > +++ b/net/mptcp/pm_netlink.c
> > > > > @@ -413,7 +413,9 @@ static int mptcp_event_created(struct
> > > > > sk_buff
> > > > > *skb,
> > > > >  	if (err)
> > > > >  		return err;
> > > > >  
> > > > > -	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
> > > > > READ_ONCE(msk-
> > > > > > pm.server_side)))
> > > > > +	/* only set when it is the server side */
> > > > > +	if (READ_ONCE(msk->pm.server_side) &&
> > > > > +	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> > > > 
> > > > In patch 2, this will be modified to use two 'if':
> > > > 
> > > > 	if (READ_ONCE(msk->pm.server_side)) {
> > > > 		flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
> > > > 
> > > > 		/* only set when it is the server side */
> > > > 		if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
> > > > 1))
> > > > 			return -EMSGSIZE;
> > > > 	}
> > > > 
> > > > Why don't we just use two 'if' statements here, which will make
> > > > patch 2
> > > > simpler:
> > > > 
> > > >        if (READ_ONCE(msk->pm.server_side)) {
> > > >                /* only set when it is the server side */
> > > >                if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> > > >                        return -EMSGSIZE;
> > > >        }
> > > > 
> > > > WDYT?
> > > 
> > > I'm not convinced: that might make the patch 2 simpler, but then
> > > the
> > > patch 1 will raise questions from reviewers and developers
> > > looking at
> > > the patches in the order they have been sent: why using 2 'if'
> > > statements while there is no need to use 2.
> > > 
> > > If we want that, I would have to add something in the commit
> > > message.
> > > I
> > > don't think that's worth it.
> > 
> > Sure. Let's keep this patch as is. If so, could you please update
> > the
> > position of the comments in patch 2 and patch 3? Something like:
> > 
> >         /* only set when it is the server side */
> >         if (READ_ONCE(msk->pm.server_side)) {
> >                 flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
> 
> Mmh, I'm not sure: for the flag, it makes sense to only set it when
> it
> is the server side, otherwise that's wrong. Keeping the comment above
> is
> then a bit confusing, no?

If this comment needs to be just above nla_put_u8(), it makes sense to
use 2 'if' here. But up to you.

Thanks,
-Geliang

> 
> > 
> >                 /* Deprecated */
> >                 if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1)) 
> >                         return -EMSGSIZE;
> >         }
> > 
> > Thanks,
> > -Geliang
> > 
> > > 
> > > Cheers,
> > > Matt
> 
> Cheers,
> Matt
Re: [PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Matthieu Baerts 6 days, 6 hours ago 
On 11/09/2025 10:45, Geliang Tang wrote:
> On Thu, 2025-09-11 at 10:37 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 11/09/2025 10:33, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Thu, 2025-09-11 at 10:20 +0200, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> Thank you for the review!
>>>>
>>>> On 11/09/2025 10:15, Geliang Tang wrote:
>>>>> Hi Matt,
>>>>>
>>>>> On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0)
>>>>> wrote:
>>>>>> This attribute is a boolean. No need to add it to set it to
>>>>>> 'false'.
>>>>>>
>>>>>> Indeed, the default value when this attribute is not set is
>>>>>> naturally
>>>>>> 'false'. A few bytes can then be saved by not adding this
>>>>>> attribute
>>>>>> if
>>>>>> the connection is not on the server side.
>>>>>>
>>>>>> This prepares the future deprecation of its attribute, in
>>>>>> favour
>>>>>> of a
>>>>>> new flag.
>>>>>>
>>>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>>>> ---
>>>>>>  Documentation/netlink/specs/mptcp_pm.yaml         | 4 ++--
>>>>>>  include/uapi/linux/mptcp_pm.h                     | 4 ++--
>>>>>>  net/mptcp/pm_netlink.c                            | 4 +++-
>>>>>>  tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
>>>>>>  4 files changed, 8 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/netlink/specs/mptcp_pm.yaml
>>>>>> b/Documentation/netlink/specs/mptcp_pm.yaml
>>>>>> index
>>>>>> d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c0304ef
>>>>>> d321
>>>>>> 5cc2
>>>>>> 4485ea22e1ede 100644
>>>>>> --- a/Documentation/netlink/specs/mptcp_pm.yaml
>>>>>> +++ b/Documentation/netlink/specs/mptcp_pm.yaml
>>>>>> @@ -28,13 +28,13 @@ definitions:
>>>>>>            traffic-patterns it can take a long time until the
>>>>>>            MPTCP_EVENT_ESTABLISHED is sent.
>>>>>>            Attributes: token, family, saddr4 | saddr6, daddr4
>>>>>> |
>>>>>> daddr6, sport,
>>>>>> -          dport, server-side, [flags].
>>>>>> +          dport, [server-side], [flags].
>>>>>>        -
>>>>>>          name: established
>>>>>>          doc: >-
>>>>>>            A MPTCP connection is established (can start new
>>>>>> subflows).
>>>>>>            Attributes: token, family, saddr4 | saddr6, daddr4
>>>>>> |
>>>>>> daddr6, sport,
>>>>>> -          dport, server-side, [flags].
>>>>>> +          dport, [server-side], [flags].
>>>>>>        -
>>>>>>          name: closed
>>>>>>          doc: >-
>>>>>> diff --git a/include/uapi/linux/mptcp_pm.h
>>>>>> b/include/uapi/linux/mptcp_pm.h
>>>>>> index
>>>>>> 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6d789
>>>>>> 6326
>>>>>> 82a9
>>>>>> bedbf8090feb9 100644
>>>>>> --- a/include/uapi/linux/mptcp_pm.h
>>>>>> +++ b/include/uapi/linux/mptcp_pm.h
>>>>>> @@ -16,10 +16,10 @@
>>>>>>   *   good time to allocate memory and send ADD_ADDR if
>>>>>> needed.
>>>>>> Depending on the
>>>>>>   *   traffic-patterns it can take a long time until the
>>>>>> MPTCP_EVENT_ESTABLISHED
>>>>>>   *   is sent. Attributes: token, family, saddr4 | saddr6,
>>>>>> daddr4
>>>>>>>
>>>>>> daddr6,
>>>>>> - *   sport, dport, server-side, [flags].
>>>>>> + *   sport, dport, [server-side], [flags].
>>>>>>   * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is
>>>>>> established
>>>>>> (can
>>>>>> start new
>>>>>>   *   subflows). Attributes: token, family, saddr4 | saddr6,
>>>>>> daddr4 |
>>>>>> daddr6,
>>>>>> - *   sport, dport, server-side, [flags].
>>>>>> + *   sport, dport, [server-side], [flags].
>>>>>>   * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped.
>>>>>> Attribute:
>>>>>> token.
>>>>>>   * @MPTCP_EVENT_ANNOUNCED: A new address has been announced
>>>>>> by
>>>>>> the
>>>>>> peer.
>>>>>>   *   Attributes: token, rem_id, family, daddr4 | daddr6 [,
>>>>>> dport].
>>>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>>>> index
>>>>>> 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b59717
>>>>>> 472a
>>>>>> ca2d
>>>>>> 38add26255419 100644
>>>>>> --- a/net/mptcp/pm_netlink.c
>>>>>> +++ b/net/mptcp/pm_netlink.c
>>>>>> @@ -413,7 +413,9 @@ static int mptcp_event_created(struct
>>>>>> sk_buff
>>>>>> *skb,
>>>>>>  	if (err)
>>>>>>  		return err;
>>>>>>  
>>>>>> -	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
>>>>>> READ_ONCE(msk-
>>>>>>> pm.server_side)))
>>>>>> +	/* only set when it is the server side */
>>>>>> +	if (READ_ONCE(msk->pm.server_side) &&
>>>>>> +	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
>>>>>
>>>>> In patch 2, this will be modified to use two 'if':
>>>>>
>>>>> 	if (READ_ONCE(msk->pm.server_side)) {
>>>>> 		flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
>>>>>
>>>>> 		/* only set when it is the server side */
>>>>> 		if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
>>>>> 1))
>>>>> 			return -EMSGSIZE;
>>>>> 	}
>>>>>
>>>>> Why don't we just use two 'if' statements here, which will make
>>>>> patch 2
>>>>> simpler:
>>>>>
>>>>>        if (READ_ONCE(msk->pm.server_side)) {
>>>>>                /* only set when it is the server side */
>>>>>                if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
>>>>>                        return -EMSGSIZE;
>>>>>        }
>>>>>
>>>>> WDYT?
>>>>
>>>> I'm not convinced: that might make the patch 2 simpler, but then
>>>> the
>>>> patch 1 will raise questions from reviewers and developers
>>>> looking at
>>>> the patches in the order they have been sent: why using 2 'if'
>>>> statements while there is no need to use 2.
>>>>
>>>> If we want that, I would have to add something in the commit
>>>> message.
>>>> I
>>>> don't think that's worth it.
>>>
>>> Sure. Let's keep this patch as is. If so, could you please update
>>> the
>>> position of the comments in patch 2 and patch 3? Something like:
>>>
>>>         /* only set when it is the server side */
>>>         if (READ_ONCE(msk->pm.server_side)) {
>>>                 flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
>>
>> Mmh, I'm not sure: for the flag, it makes sense to only set it when
>> it
>> is the server side, otherwise that's wrong. Keeping the comment above
>> is
>> then a bit confusing, no?
> 
> If this comment needs to be just above nla_put_u8(), it makes sense to
> use 2 'if' here. But up to you.

Sorry, I'm lost:

- the comment above is for nla_put_u8(): that's strange to have such
  comment with a flag that is obviously only set for the server side

- we need 2 'if' because there are two different actions here: set flags
  + return error if any, no?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-next 1/4] mptcp: pm: netlink: only add server-side attr when true
Posted by Geliang Tang 6 days, 6 hours ago 
On Thu, 2025-09-11 at 11:28 +0200, Matthieu Baerts wrote:
> On 11/09/2025 10:45, Geliang Tang wrote:
> > On Thu, 2025-09-11 at 10:37 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 11/09/2025 10:33, Geliang Tang wrote:
> > > > Hi Matt,
> > > > 
> > > > On Thu, 2025-09-11 at 10:20 +0200, Matthieu Baerts wrote:
> > > > > Hi Geliang,
> > > > > 
> > > > > Thank you for the review!
> > > > > 
> > > > > On 11/09/2025 10:15, Geliang Tang wrote:
> > > > > > Hi Matt,
> > > > > > 
> > > > > > On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0)
> > > > > > wrote:
> > > > > > > This attribute is a boolean. No need to add it to set it
> > > > > > > to
> > > > > > > 'false'.
> > > > > > > 
> > > > > > > Indeed, the default value when this attribute is not set
> > > > > > > is
> > > > > > > naturally
> > > > > > > 'false'. A few bytes can then be saved by not adding this
> > > > > > > attribute
> > > > > > > if
> > > > > > > the connection is not on the server side.
> > > > > > > 
> > > > > > > This prepares the future deprecation of its attribute, in
> > > > > > > favour
> > > > > > > of a
> > > > > > > new flag.
> > > > > > > 
> > > > > > > Signed-off-by: Matthieu Baerts (NGI0)
> > > > > > > <matttbe@kernel.org>
> > > > > > > ---
> > > > > > >  Documentation/netlink/specs/mptcp_pm.yaml         | 4
> > > > > > > ++--
> > > > > > >  include/uapi/linux/mptcp_pm.h                     | 4
> > > > > > > ++--
> > > > > > >  net/mptcp/pm_netlink.c                            | 4
> > > > > > > +++-
> > > > > > >  tools/testing/selftests/net/mptcp/userspace_pm.sh | 2 +-
> > > > > > >  4 files changed, 8 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > > > b/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > > > index
> > > > > > > d1b4829b580ad09baf4afd73b67abd7b4ef6883a..fc47a2931014c03
> > > > > > > 04ef
> > > > > > > d321
> > > > > > > 5cc2
> > > > > > > 4485ea22e1ede 100644
> > > > > > > --- a/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > > > +++ b/Documentation/netlink/specs/mptcp_pm.yaml
> > > > > > > @@ -28,13 +28,13 @@ definitions:
> > > > > > >            traffic-patterns it can take a long time until
> > > > > > > the
> > > > > > >            MPTCP_EVENT_ESTABLISHED is sent.
> > > > > > >            Attributes: token, family, saddr4 | saddr6,
> > > > > > > daddr4
> > > > > > > > 
> > > > > > > daddr6, sport,
> > > > > > > -          dport, server-side, [flags].
> > > > > > > +          dport, [server-side], [flags].
> > > > > > >        -
> > > > > > >          name: established
> > > > > > >          doc: >-
> > > > > > >            A MPTCP connection is established (can start
> > > > > > > new
> > > > > > > subflows).
> > > > > > >            Attributes: token, family, saddr4 | saddr6,
> > > > > > > daddr4
> > > > > > > > 
> > > > > > > daddr6, sport,
> > > > > > > -          dport, server-side, [flags].
> > > > > > > +          dport, [server-side], [flags].
> > > > > > >        -
> > > > > > >          name: closed
> > > > > > >          doc: >-
> > > > > > > diff --git a/include/uapi/linux/mptcp_pm.h
> > > > > > > b/include/uapi/linux/mptcp_pm.h
> > > > > > > index
> > > > > > > 7359d34da446b94be148b363079120db03ba8549..bf44a5cf5b5a1e6
> > > > > > > d789
> > > > > > > 6326
> > > > > > > 82a9
> > > > > > > bedbf8090feb9 100644
> > > > > > > --- a/include/uapi/linux/mptcp_pm.h
> > > > > > > +++ b/include/uapi/linux/mptcp_pm.h
> > > > > > > @@ -16,10 +16,10 @@
> > > > > > >   *   good time to allocate memory and send ADD_ADDR if
> > > > > > > needed.
> > > > > > > Depending on the
> > > > > > >   *   traffic-patterns it can take a long time until the
> > > > > > > MPTCP_EVENT_ESTABLISHED
> > > > > > >   *   is sent. Attributes: token, family, saddr4 |
> > > > > > > saddr6,
> > > > > > > daddr4
> > > > > > > > 
> > > > > > > daddr6,
> > > > > > > - *   sport, dport, server-side, [flags].
> > > > > > > + *   sport, dport, [server-side], [flags].
> > > > > > >   * @MPTCP_EVENT_ESTABLISHED: A MPTCP connection is
> > > > > > > established
> > > > > > > (can
> > > > > > > start new
> > > > > > >   *   subflows). Attributes: token, family, saddr4 |
> > > > > > > saddr6,
> > > > > > > daddr4 |
> > > > > > > daddr6,
> > > > > > > - *   sport, dport, server-side, [flags].
> > > > > > > + *   sport, dport, [server-side], [flags].
> > > > > > >   * @MPTCP_EVENT_CLOSED: A MPTCP connection has stopped.
> > > > > > > Attribute:
> > > > > > > token.
> > > > > > >   * @MPTCP_EVENT_ANNOUNCED: A new address has been
> > > > > > > announced
> > > > > > > by
> > > > > > > the
> > > > > > > peer.
> > > > > > >   *   Attributes: token, rem_id, family, daddr4 | daddr6
> > > > > > > [,
> > > > > > > dport].
> > > > > > > diff --git a/net/mptcp/pm_netlink.c
> > > > > > > b/net/mptcp/pm_netlink.c
> > > > > > > index
> > > > > > > 483ddbb9ec406a3e965376ee5a833ae295896a02..33a6bf536c020b5
> > > > > > > 9717
> > > > > > > 472a
> > > > > > > ca2d
> > > > > > > 38add26255419 100644
> > > > > > > --- a/net/mptcp/pm_netlink.c
> > > > > > > +++ b/net/mptcp/pm_netlink.c
> > > > > > > @@ -413,7 +413,9 @@ static int mptcp_event_created(struct
> > > > > > > sk_buff
> > > > > > > *skb,
> > > > > > >  	if (err)
> > > > > > >  		return err;
> > > > > > >  
> > > > > > > -	if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
> > > > > > > READ_ONCE(msk-
> > > > > > > > pm.server_side)))
> > > > > > > +	/* only set when it is the server side */
> > > > > > > +	if (READ_ONCE(msk->pm.server_side) &&
> > > > > > > +	    nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE, 1))
> > > > > > 
> > > > > > In patch 2, this will be modified to use two 'if':
> > > > > > 
> > > > > > 	if (READ_ONCE(msk->pm.server_side)) {
> > > > > > 		flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
> > > > > > 
> > > > > > 		/* only set when it is the server side */
> > > > > > 		if (nla_put_u8(skb,
> > > > > > MPTCP_ATTR_SERVER_SIDE,
> > > > > > 1))
> > > > > > 			return -EMSGSIZE;
> > > > > > 	}
> > > > > > 
> > > > > > Why don't we just use two 'if' statements here, which will
> > > > > > make
> > > > > > patch 2
> > > > > > simpler:
> > > > > > 
> > > > > >        if (READ_ONCE(msk->pm.server_side)) {
> > > > > >                /* only set when it is the server side */
> > > > > >                if (nla_put_u8(skb, MPTCP_ATTR_SERVER_SIDE,
> > > > > > 1))
> > > > > >                        return -EMSGSIZE;
> > > > > >        }
> > > > > > 
> > > > > > WDYT?
> > > > > 
> > > > > I'm not convinced: that might make the patch 2 simpler, but
> > > > > then
> > > > > the
> > > > > patch 1 will raise questions from reviewers and developers
> > > > > looking at
> > > > > the patches in the order they have been sent: why using 2
> > > > > 'if'
> > > > > statements while there is no need to use 2.
> > > > > 
> > > > > If we want that, I would have to add something in the commit
> > > > > message.
> > > > > I
> > > > > don't think that's worth it.
> > > > 
> > > > Sure. Let's keep this patch as is. If so, could you please
> > > > update
> > > > the
> > > > position of the comments in patch 2 and patch 3? Something
> > > > like:
> > > > 
> > > >         /* only set when it is the server side */
> > > >         if (READ_ONCE(msk->pm.server_side)) {
> > > >                 flags |= MPTCP_PM_EV_FLAG_SERVER_SIDE;
> > > 
> > > Mmh, I'm not sure: for the flag, it makes sense to only set it
> > > when
> > > it
> > > is the server side, otherwise that's wrong. Keeping the comment
> > > above
> > > is
> > > then a bit confusing, no?
> > 
> > If this comment needs to be just above nla_put_u8(), it makes sense
> > to
> > use 2 'if' here. But up to you.
> 
> Sorry, I'm lost:
> 
> - the comment above is for nla_put_u8(): that's strange to have such
>   comment with a flag that is obviously only set for the server side
> 
> - we need 2 'if' because there are two different actions here: set
> flags
>   + return error if any, no?

Sorry for the confusion, let's keep them as is.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.