Re: [PATCH mptcp-next v2] Squash-to "mptcp: pm: init and release mptcp_pm_ops"

[gang.yan@linux.dev]

July 2, 2026 at 10:15 AM, "Geliang Tang" <geliang@kernel.org mailto:geliang@kernel.org?to=%22Geliang%20Tang%22%20%3Cgeliang%40kernel.org%3E > wrote:


Hi Geliang,

Thanks for your review.

> 
> Hi Gang,
> 
> Thanks for this v2.
> 
> On Wed, 2026-07-01 at 15:37 +0800, Gang Yan wrote:
> 
> > 
> > From: Gang Yan <yangang@kylinos.cn>
> >  
> >  This patch removes the need to handle a NULL pm_ops in the fallback
> >  path, and moves the rcu_read_lock\unlock into mptcp_pm_ops_init.
> >  
> >  Following sashiko's comments, it also moves the pr_debug() before
> >  bpf_module_put() in mptcp_pm_ops_release.
> >  
> >  Signed-off-by: Gang Yan <yangang@kylinos.cn>
> >  ---
> >   net/mptcp/pm.c | 19 ++++++++++---------
> >   1 file changed, 10 insertions(+), 9 deletions(-)
> >  
> >  diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >  index 9dc7b41fb562..5fe4276cf895 100644
> >  --- a/net/mptcp/pm.c
> >  +++ b/net/mptcp/pm.c
> >  @@ -1137,12 +1137,14 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
> >    spin_unlock_bh(&msk->pm.lock);
> >   }
> >   
> >  -static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> >  -       struct mptcp_pm_ops *pm_ops)
> >  +static void mptcp_pm_ops_init(struct mptcp_sock *msk, const char
> >  *pm_name)
> >   {
> >  + struct mptcp_pm_ops *pm_ops;
> >  +
> >  + rcu_read_lock();
> >  + pm_ops = mptcp_pm_find(pm_name);
> >    if (!pm_ops || !bpf_try_module_get(pm_ops, pm_ops->owner)) {
> >  - pr_warn_once("pm %s fails, fallback to default pm",
> >  -      pm_ops->name);
> >  + pr_warn_once("pm %s fails, fallback to default pm",
> >  pm_name);
> >    pm_ops = &mptcp_pm_kernel;
> >    }
> >   
> >  @@ -1151,6 +1153,7 @@ static void mptcp_pm_ops_init(struct mptcp_sock
> >  *msk,
> >    msk->pm.ops->init(msk);
> >   
> >    pr_debug("pm %s initialized\n", pm_ops->name);
> > 
> nit: Here we can also print "pm_name" to keep it consistent with
> pr_warn_once().

Here is the print of the name of initialized pm, for example, if we fallback
to kernel_PM it will print like:

'''
pm bpf_pm fails, fallback to default pm
pm kernel initialized
'''

So I think it should use 'pm_ops->name' here.
WDYT?
> 
> > 
> > + rcu_read_unlock();
> > 
> Then this unlock line can be moved ahead of the previous pr_debug()
> line.

> 
> > 
> > }
> >   
> >   static void mptcp_pm_ops_release(struct mptcp_sock *msk)
> >  @@ -1161,9 +1164,9 @@ static void mptcp_pm_ops_release(struct
> >  mptcp_sock *msk)
> >    if (pm_ops->release)
> >    pm_ops->release(msk);
> >   
> >  - bpf_module_put(pm_ops, pm_ops->owner);
> >  -
> >    pr_debug("pm %s released\n", pm_ops->name);
> >  +
> >  + bpf_module_put(pm_ops, pm_ops->owner);
> >   }
> >   
> >   void mptcp_pm_destroy(struct mptcp_sock *msk)
> >  @@ -1184,9 +1187,7 @@ void mptcp_pm_data_reset(struct mptcp_sock
> >  *msk)
> >    pm->rm_list_rx.nr = 0;
> >    WRITE_ONCE(pm->pm_type, pm_type);
> >   
> >  - rcu_read_lock();
> >  - mptcp_pm_ops_init(msk, mptcp_pm_find(pm_name));
> >  - rcu_read_unlock();
> > 
> Or we leave rcu_read_lock() and rcu_read_unlock() where they are,
> without moving them into mptcp_pm_ops_init().
> 
LGTM, I'll apply the reset suggestions when sending v3.
> Thanks,
> -Geliang
> 
> > 

Thanks
Gang

> > + mptcp_pm_ops_init(msk, pm_name);
> >   }
> >   
> >   void mptcp_pm_data_init(struct mptcp_sock *msk)
> >
>