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