Re: [PATCH mptcp-next v2 03/36] mptcp: add mptcp_for_each_address macros

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Similar to mptcp_for_each_subflow() and mptcp_for_each_subflow_safe()
> macros, this patch adds two new macros mptcp_for_each_address() and
> mptcp_for_each_address_safe() to iterate over the address entries on
> userspace_pm_local_addr_list of the mptcp socket.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_userspace.c | 12 ++++++------
>  net/mptcp/protocol.h     |  5 +++++
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 2cceded3a83a..00a7f9dd90cf 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -41,7 +41,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  	bitmap_zero(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	list_for_each_entry(e, &msk->pm.userspace_pm_local_addr_list, list) {
> +	mptcp_for_each_address(msk, e) {
>  		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
>  		if (addr_match && entry->addr.id == 0 && needs_id)
>  			entry->addr.id = e->addr.id;
> @@ -92,7 +92,7 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  {
>  	struct mptcp_pm_addr_entry *entry, *tmp;
>  
> -	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> +	mptcp_for_each_address_safe(msk, entry, tmp) {
>  		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
>  			/* TODO: a refcount is needed because the entry can
>  			 * be used multiple times (e.g. fullmesh mode).
> @@ -112,7 +112,7 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
>  {
>  	struct mptcp_pm_addr_entry *entry;
>  
> -	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> +	mptcp_for_each_address(msk, entry) {
>  		if (entry->addr.id == id)
>  			return entry;
>  	}
> @@ -127,7 +127,7 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
>  			     inet_sk((struct sock *)msk))->inet_sport;
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	list_for_each_entry(e, &msk->pm.userspace_pm_local_addr_list, list) {
> +	mptcp_for_each_address(msk, e) {
>  		if (mptcp_addresses_equal(&e->addr, skc, false)) {
>  			entry = e;
>  			break;
> @@ -155,7 +155,7 @@ bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
>  	bool backup = false;
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> +	mptcp_for_each_address(msk, entry) {
>  		if (mptcp_addresses_equal(&entry->addr, skc, false)) {
>  			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>  			break;
> @@ -642,7 +642,7 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  
>  	lock_sock(sk);
>  	spin_lock_bh(&msk->pm.lock);
> -	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> +	mptcp_for_each_address(msk, entry) {
>  		if (test_bit(entry->addr.id, bitmap->map))
>  			continue;
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index b4c72a73594f..ce3b12778b3f 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -354,6 +354,11 @@ struct mptcp_sock {
>  #define mptcp_for_each_subflow_safe(__msk, __subflow, __tmp)			\
>  	list_for_each_entry_safe(__subflow, __tmp, &((__msk)->conn_list), node)
>  
> +#define mptcp_for_each_address(__msk, __entry)			\
> +	list_for_each_entry(__entry, &((__msk)->pm.userspace_pm_local_addr_list), list)
> +#define mptcp_for_each_address_safe(__msk, __entry, __tmp)			\
> +	list_for_each_entry_safe(__entry, __tmp, &((__msk)->pm.userspace_pm_local_addr_list), list)

If it is specific to the userspace PM, maybe best to declare them in
pm_userspace.c, no?

Also, the name is very generic for something so specific. Maybe:

  mptcp_for_each_pm_userspace_local_addr(_safe)

Otherwise, if we see "mptcp_for_each_address(msk, entry)" in the code,
it is really not clear what you are looking at I think, no?

> +
>  extern struct genl_family mptcp_genl_family;
>  
>  static inline void msk_owned_by_me(const struct mptcp_sock *msk)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.