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

[Mat Martineau <martineau@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

On Fri, 25 Oct 2024, Matthieu Baerts wrote:

> On 25/10/2024 11:32, 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..f38e1ccd34e95cd88b179a8b50e6965731542871 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -520,12 +520,12 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
>>  }
>>
>>  static struct mptcp_pm_addr_entry *
>> -__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
>> +__lookup_addr_by_id_rcu(struct pm_nl_pernet *pernet, unsigned int id)
>>  {
>>  	struct mptcp_pm_addr_entry *entry;
>>
>> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
>> -		if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port))
>> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
>
> I didn't see the following in the RCU doc -- but it is quite big, so I
> probably missed it -- but I suppose we don't need to keep the two
> helpers here with and without the _rcu variant (__lookup_addr_by_id())
> if here we add an extra lockdep condition:
>
>  list_for_each_entry_rcu(entry, &pernet->local_addr_list, list,
>                          lockdep_is_held(&pernet->lock)) {
>
> WDYT?

I like this idea, would clean things up.

>
>> +		if (entry->addr.id == id)
>>  			return entry;
>>  	}
>>  	return NULL;
>
> (...)
>
>> @@ -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];
>
> Oops, I left my temp declaration, it should be the following line, which
> will result in the same thing anyway on x86_64, no need to re-run the
> tests → I can fix that when applying the patches, or in a future version:
>
>  DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
>

Thanks for catching that. I think it would be best to have a v3 with the 
lockdep_is_held() approach you mentioned above.


- Mat