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

[Geliang Tang <geliang@kernel.org>]

On Wed, 2024-10-30 at 19:20 +0100, Matthieu Baerts wrote:
> 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?

Good idea! I moved it into pm_userspace.c and renamed it as
mptcp_for_each_userspace_pm_addr.

Furthermore, I renamed mptcp_address bpf_iter in another set "add
mptcp_address bpf_iter" as mptcp_userspace_pm_addr bpf_iter to keep it
consistent with this patch.

Thanks,
-Geliang

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