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

[Geliang Tang <geliang@kernel.org>]

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