Re: [PATCH mptcp-next v2 7/8] mptcp: add mptcp_pm_addr_id_bitmap_t type

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Mon, 2024-12-30 at 17:30 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 13/12/2024 08:35, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Similar to defining types such as nodemask_t, dma_cap_mask_t and
> >    A. > > cpumask_t to simplify the use of bitmap, a new type for
> >       MPTCP
> > userspace pm id bitmap, mptcp_pm_addr_id_bitmap_t is defined to
> > easily modify dump_addr() interface of the path managers to accept
> > an mptcp_pm_addr_id_bitmap_t type parameter. It also allows this
> > parameter of dump_addr() can be modified by BPF program when
> > implementing this interface of a BFP path manager. Because a
> > dump_addr() interface that accepts an 'unsigned long *bitmap'
> > or 'unsigned long bitmap[]' parameter is difficult to implement
> > in BPF program.
> > 
> > In addition, this also makes it easier for us to implement similar
> > logic to mptcp_userspace_pm_append_new_local_addr() in BPF path
> > manager, because there's no way to use DECLARE_BITMAP macro in BPF
> > program, and it's not easy to reimplement it in BPF.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  include/net/mptcp.h      |  7 +++++++
> >  net/mptcp/pm_userspace.c | 14 ++++++--------
> >  net/mptcp/protocol.h     |  3 ---
> >  3 files changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index 814b5f2e3ed5..220b1f60e8c1 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -120,6 +120,13 @@ struct mptcp_sched_ops {
> >   void (*release)(struct mptcp_sock *msk);
> >  } ____cacheline_aligned_in_smp;
> >  
> > +/* max value of mptcp_addr_info.id */
> > +#define MPTCP_PM_MAX_ADDR_ID U8_MAX
> > +
> > +typedef struct {
> > + DECLARE_BITMAP(map, MPTCP_PM_MAX_ADDR_ID + 1);
> > +} mptcp_pm_addr_id_bitmap_t;
> 
> Why do you need to declare it here and not in protocol.h? Is it for
> the
> future BPF PM?

Yes, it's for the BPF PM. If we define a struct mptcp_pm_addr_id_bitmap
as in the previous version [1], we can indeed put it in protocol.h:

struct mptcp_pm_addr_id_bitmap {
DECLARE_BITMAP(map, MPTCP_PM_MAX_ADDR_ID + 1);
};

and just declare it in include/net/mptcp.h:

struct mptcp_pm_addr_id_bitmap;

But as your comments in [1], 'why do you need a
specific structure with only one field?'

This new version uses 'typedef' to define a new type, similar to
nodemask_t, dma_cap_mask_t and cpumask_t. I think this is a good way to
handle structures with only one field. But on the other hand, we can't
declare a 'mptcp_pm_addr_id_bitmap_t' in include/net/mptcp.h, we need
to move the entire typedef to include/net/mptcp.h, and the
corresponding MPTCP_PM_MAX_ADDR_ID also needs to be moved together.

Which one do you prefer, mptcp_pm_addr_id_bitmap_t or struct
mptcp_pm_addr_id_bitmap? I will use it in the next version.

Thanks,
-Geliang

[1]https://patchwork.kernel.org/project/mptcp/patch/344964d66ab0250ae027ffd46a0e6782d572c33d.1729588019.git.tanggeliang@kylinos.cn/

> 
> Maybe you don't really need this bitmap if it is only needed for the
> dump interfaces that are probably not needed with BPF?
> 
> Then maybe there is no need to change anything here?
> 
> Cheers,
> Matt