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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 09/01/2025 05:15, Geliang Tang wrote:
> 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.

My question was more to know why BPF is not happy if the struct or
typedef is declared in net/mptcp/protocol.h instead of include/net/mptcp.h.

If really it has to be declared in 'include', then maybe better to add
the minimum (struct mptcp_pm_addr_id_bitmap;) and the typedef in protocol.h.

>> 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?

Regarding these two questions: why do you need the bitmap? If I'm not
mistaken, the BPF PM doesn't need to implement the 'dump' and 'get_addr'
interfaces: no need to expose this to userspace via netlink if the PM is
handled in kernelspace with BPF, no?

A BPF PM can expose what it wants via BPF.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.