Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Fri, 2024-10-25 at 17:26 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 25/10/2024 16:25, Geliang Tang wrote:
> > On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
> > > In a few places -- to get an endpoint, dump all of them, and
> > > change
> > > their flags -- the list is iterated while holding the pernet-
> > > >lock,
> > > but
> > > only to read the content of the list. In these cases, we can
> > > replace
> > > the
> > > spin locks, by RCU read ones, and use the _rcu variants to
> > > iterate
> > > over
> > > the entries list in a lockless way.
> > > 
> > > To make it clear, the lookup helpers using the _rcu variant are
> > > renamed
> > > with a _rcu suffix. The previous __lookup_addr() helper can then
> > > be
> > > removed, but __lookup_addr_by_id() is still needed.
> > > 
> > > While at it, the IDs bitmap is copied before iterating the list
> > > to
> > > dump
> > > the different addresses, to avoid any consistencies.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > Notes:
> > >  - This is not a fix, a small improvement for -next.
> > > ---
> > >  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
> > >  1 file changed, 19 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index
> > > a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8
> > > b50e
> > > 6965731542871 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> 
> (...)
> 
> > > @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff
> > > *msg,
> > >  	struct net *net = sock_net(msg->sk);
> > >  	struct mptcp_pm_addr_entry *entry;
> > >  	struct pm_nl_pernet *pernet;
> > > +	unsigned long id_bitmap[4];
> > >  	int id = cb->args[0];
> > >  	void *hdr;
> > >  	int i;
> > >  
> > >  	pernet = pm_nl_get_pernet(net);
> > > +	bitmap_copy(id_bitmap, pernet->id_bitmap,
> > > MPTCP_PM_MAX_ADDR_ID + 1);
> > 
> > I think the id bitmap should only be copied when id is 0:
> > 
> > if (!id)
> >   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID +
> > 1);
> > 
> > Since this mptcp_pm_nl_dump_addr() may be called repeatedly when
> > the
> > dump information is very long. We only copy it the first time it is

A correction is needed here. Regardless of whether the dump information
is large or small, the dumpit function will be called repeatedly when
dumpit returns non-zero. The loop stops when dumpit returns 0.

> > called, and subsequent calls cannot copy it again. WDYT?
> 
> Sorry, I'm not sure to understand. Here, I did a local copy of
> 'id_bitmap' just to keep a certain consistency if the bitmap is
> changed
> during the RCU read section below: not to have this bitmap being
> changed
> during the for loop here below. Before, we had this protection
> because
> pernet->lock was held.

It is precisely because we need to maintain this consistency that we
need to copy the bitmap only once. If we copy the bitmap when dumpit is
called a second time, the bitmap obtained at this time is a new bitmap,
which destroys this consistency, right?

> 
> Are you suggesting here to keep a copy in the cb, not a local one, to
> do
> the copy only once? Are you sure it is worth it (and safe)?

Because dumpit will be called repeatedly, we need to ensure that the
same bitmap is accessed each time, so we cannot use a local one, but
need to keep a copy in the cb just like what we do in
mptcp_userspace_pm_dump_addr.


In addition, when doing bitmap_copy, we need to hold pernet->lock:

bitmap = (struct mptcp_id_bitmap *)cb->ctx;

if (!id) {
  spin_lock_bh(&pernet->lock);
  bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
  spin_unlock_bh(&pernet->lock);
}

Also, we can put this bitmap copy code into a separate patch, which is
applied before this one. In this patch we only need to replace
spin_lock_bh with rcu_read_lock.

WDYT?

Thanks,
-Geliang

> 
> > >  
> > > -	spin_lock_bh(&pernet->lock);
> > > +	rcu_read_lock();
> > >  	for (i = id; i < MPTCP_PM_MAX_ADDR_ID + 1; i++) {
> > > -		if (test_bit(i, pernet->id_bitmap)) {
> > > -			entry = __lookup_addr_by_id(pernet, i);
> > > +		if (test_bit(i, id_bitmap)) {
> > > +			entry = __lookup_addr_by_id_rcu(pernet,
> > > i);
> > >  			if (!entry)
> > >  				break;
> > >  
> Cheers,
> Matt