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

[Matthieu Baerts <matttbe@kernel.org>]

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..f38e1ccd34e95cd88b179a8b50e
>> 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
> 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.

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)?

>>  
>> -	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
-- 
Sponsored by the NGI0 Core fund.