Re: [PATCH mptcp-next v2 13/36] mptcp: add struct mptcp_id_bitmap

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2024-10-31 at 19:45 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/10/2024 11:14, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > A new struct mptcp_id_bitmap is defined to unify all bitmap type of
> > address
> > IDs for both in-kernel PM and userspace PM.
> > 
> > This type can be used to easily refactor dump_addr() interface of
> > the path
> > managers to accept an mptcp_id_bitmap 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.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> 
> (...)
> 
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 8282a6355765..4358a9a21270 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -211,6 +211,10 @@ enum mptcp_addr_signal_status {
> >  /* max value of mptcp_addr_info.id */
> >  #define MPTCP_PM_MAX_ADDR_ID		U8_MAX
> >  
> > +struct mptcp_id_bitmap {
> 
> This name is too generic, while here this bitmap is specific to the
> path-manager and its PM address:
> 
>   mptcp_pm_addr_avail_id?
> 
> > +	DECLARE_BITMAP(map, MPTCP_PM_MAX_ADDR_ID + 1);
> 
> Sorry, I'm not sure to understand this modification: why do you need
> a
> specific structure with only one field? I didn't check the next
> patches
> yet, but I saw that at the end of the series, the structure still
> only
> has one field.
> 
> Do you require this dedicated structure because of a limitation of
> BPF?

Yes, defining dump_addr interface as either

int userspace_pm_dump_addr(struct mptcp_sock *msk,
                           unsigned long *bitmap)

or

int userspace_pm_dump_addr(struct mptcp_sock *msk,
			   unsigned long id_bitmap[4])

is not allowed by BPF.

> Can we not use a typedef, a macro or something else to avoid all
> these
> modifications here?

In v3 I defined a type using typedef like this:

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

and only used it in the parameters of the dump_addr interfaces. The
rest of the places continue to use DECLARE_BITMAP. It works well, the
only downside is that checkpatch.pl complains about it:

$ ./scripts/checkpatch.pl 0001-mptcp-add-mptcp_pm_addr_id_bitmap_t-
type.patch 
WARNING: do not add new typedefs
#31: FILE: include/net/mptcp.h:124:
+typedef struct {

total: 0 errors, 1 warnings, 0 checks, 40 lines checked

We can ignore this warning.

WDYT?

Thanks,
-Geliang

> 
> > +};
> > +
> >  struct mptcp_pm_data {
> >  	struct mptcp_addr_info local;
> >  	struct mptcp_addr_info remote;
> > @@ -231,7 +235,7 @@ struct mptcp_pm_data {
> >  	u8		pm_type;
> >  	u8		subflows;
> >  	u8		status;
> > -	DECLARE_BITMAP(id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
> > +	struct mptcp_id_bitmap id_avail_bitmap;
> >  	struct mptcp_rm_list rm_list_tx;
> >  	struct mptcp_rm_list rm_list_rx;
> >  };
> 
> Cheers,
> Matt