diff mbox series

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

Message ID b6b53fd730a0edd0024963468d078488ad5e67aa.1729588019.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series BPF path manager | expand

Checks

Context Check Description
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 59 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build warning Build error with: make C=1 net/mptcp/bpf.o
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang Oct. 22, 2024, 9:14 a.m. UTC 
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(-)

Comments

Matthieu Baerts (NGI0) Oct. 30, 2024, 6:20 p.m. UTC | #1 
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
Geliang Tang Oct. 31, 2024, 7:55 a.m. UTC | #2 
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
diff mbox series

Patch

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)
+
 extern struct genl_family mptcp_genl_family;
 
 static inline void msk_owned_by_me(const struct mptcp_sock *msk)