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

[Matthieu Baerts <matttbe@kernel.org>]

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?
Can we not use a typedef, a macro or something else to avoid all these
modifications here?

> +};
> +
>  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
-- 
Sponsored by the NGI0 Core fund.