Re: [PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2025-01-09 at 13:20 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your reply!
> 
> On 09/01/2025 04:40, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for the review!
> > 
> > On Wed, 2025-01-08 at 19:51 +0100, Matthieu Baerts wrote:
> > > On 08/01/2025 19:47, Matthieu Baerts wrote:
> > > > Hi Geliang,
> > > > 
> > > > On 08/01/2025 05:21, Geliang Tang wrote:
> > > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > 
> > > > > Similar to in-kernel PM, this patch adds address ID support
> > > > > to
> > > > > set_flags()
> > > > > interface of userspace PM, allowing it to work with either an
> > > > > address or
> > > > > an address ID.
> > > > > 
> > > > > When an address ID is used,
> > > > > mptcp_userspace_pm_lookup_addr_by_id() helper
> > > > > is used to look up the address entry in the local address
> > > > > list
> > > > > instead of
> > > > > using mptcp_userspace_pm_lookup_addr().
> > > > 
> > > > Mmh, I'm still not sure about that. As I was saying in [1], if
> > > > I'm
> > > > not
> > > > mistaken, with the userspace PM, it is possible not to find any
> > > > entries
> > > > here, e.g.: if a subflow using this address has not been added
> > > > or
> > > > the
> > > > address has not been announced. (I guess the initial address is
> > > > not
> > > > there then).
> > 
> > The previous version (in [1]) did have this issue and
> > userspace_pm.sh
> > tests would fail because of it, but this new version has fixed it.
> > 
> >  mptcp_pm_nl_mp_prio_send_ack(msk,
> >                               entry ? &entry->addr : &local->addr,
> >                               remote, bkup);
> > 
> > When the entry is not found, we continue to pass local->addr to
> > ensure
> > the same behavior as before.
> 
> Yes indeed, the tests are fixed, but if 'entry' is NULL, the address
> you
> will give will be empty, so it will not be able to find any subflow
> to
> send the MP_PRIO, right?
> 
> > > > Do you think this patch is worth it? Setting by ID for the in-
> > > > kernel PM
> > > > makes sense: unique ID for the netns, easier to type the ID
> > > > than
> > > > the
> > > > full address. While for the userspace PM, it will be managed by
> > > > a
> > > > daemon
> > > > that will have to track addresses anyway.
> > 
> > I think it's still useful to extend this functionality while the
> > original behavior is not affected, at least it doesn't hurt.
> 
> I'm sorry, I think it is not that simple: if we extend this
> functionality, it means we will have to maintain it. Here, the
> interface
> looks buggy because it will not work with all addresses: the initial
> ones, the ones not announced but implicitly used, etc.
> 
> If the interface does not always work, I don't think we will
> recommend
> using it, then why do we need to maintain it?
> 
> > We cannot assume that userspace PM is always managed by a daemon.
> > We
> > have exported its interfaces to BPF. We allow users to customize
> > path
> > managers. That means we also allow users to use their own userspace
> > PM
> > in any way.
> 
> I think the BPF PM is different: it is a different interface.
> 
> To interact with the userspace PM, it is required to monitor the
> MPTCP
> events sent via Netlink, e.g. to get the token. When a new subflow is
> created, the userspace will know which addresses (including the ID)
> it
> is linked to. In this case, why only setting the ID in the address
> structure if it doesn't always work, while setting the address will
> always work as expected.
> 
> > Another consideration is that we need to maintain the consistency
> > between in-kernel PM and userspace PM.
> 
> Not really: when they can do the same thing, yes, but the two
> interfaces
> are different. We don't have to keep the consistency if it doesn't
> make
> sense to do so.
> 
> > For ease of maintenance, we need
> > to make these two PMs use the same code as much as possible, and
> > only
> > abstract their differences through PM interfaces such as get_addr,
> > dump_addr, set_flags, etc.
> 
> Yes but there are some limits: if some code is shared between
> multiple
> interfaces, it is important not to break one of them when changing
> the
> code. In other words, if the behaviour is very similar (e.g.
> get_addr),
> that's fine. But if they start to be too different, you have complex
> common code where you need to think "OK, this one acts like that, but
> the other one like that", and complexity is not good for the
> maintenance. In this case, it sounds better to keep them separated.
> 
> > At present, the biggest difference between
> > the two is that they use different linked lists (pernet-
> > > local_addr_list vs. msk->pm.userspace_pm_local_addr_list) to
> > > store
> > address entries, so we only need to put the code for operating the
> > linked lists into the interfaces of each PM. This is also the goal
> > of
> > adjusting the pm interfaces in this series.
> 
> Yes, but that's not the only difference, because the interfaces are
> different.
> 
> With the in-kernel PM, we act per netns, while with the userspace PM,
> it
> is per connection. Because of that, addresses lists are managed
> differently, leading to different concept, e.g. the list not having
> all
> addresses, the addresses not having ID 0 in one, but OK in the other,
> etc. With shared code that acts for both of them, you need to keep
> thinking about these differences when reading or writing code, and
> that's a source of error I think.
> 
> > > > Or in other words, do you have a use-case for this? To me, it
> > > > looks
> > > > like
> > > > "yes, you can only set the ID, but it might not always work".
> > > > Then
> > > > maybe
> > > > better to always set the full address, no?
> > 
> > If you're worried that this functionality isn't covered by tests,
> > I've
> > added a test that covers it in BPF path manager selftests:
> > 
> >         err = userspace_pm_set_flags(token, addr, "backup");
> >         if (!ASSERT_OK(err, "userspace_pm_set_flags backup"))
> >                 goto close_accept;
> > 
> > 	...
> > 
> >         err = userspace_pm_set_flags_by_id(token, 100, "nobackup");
> >         if (!ASSERT_OK(err, "userspace_pm_set_flags_by_id
> > nobackup"))
> >                 goto close_accept;
> 
> I would need to check the BPF PM interface, but for me the userspace
> PM
> and BPF PM interfaces don't have to be the same, e.g. why having a
> dump
> if the BPF PM can directly access data from the kernel? Same here for
> the ID: it depends if all IDs are tracked in the corresponding list,
> e.g. it might not be the case with an "announced" list.
> 
> But also yes, if something is exposed to userspace (via Netlink), it
> should be covered by a test (using the userspace Netlink interface)
> 
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/mptcp/d01d0e8a-5606-4152-aabe-32e4402adeeb@kernel.org/
> > > 
> > > Note: if we drop this patch (I think it is better), maybe patch
> > > 8/8
> > > is
> > > not worth it: not to have a "common" section with plenty of 'if
> > > (token)', no? Or do you really need them for the BPF PM?
> > 
> > Here we are only adjusting set_flags interface of in-kernel PM and
> > userspace PM, which has nothing to do with the BPF PM
> > implementation.
> > 
> > It seems that moving the code in mptcp_pm_nl_set_flags_doit() to
> > mptcp_pm_set_flags() can remove these 'if (token)':
> > 
> > int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct
> > genl_info
> > *info)
> > {
> >         return mptcp_pm_set_flags(info);
> 
> I'm not sure whether it is useful to have one function simply calling
> another function that is only used once.

I kept this function in v4 to make it consistent with mptcp_pm_get_addr
and mptcp_pm_dump_addr.

And "mptcp: userspace pm set_flags id support" is moved out of this set
in v4.

Thanks,
-Geliang

> 
> > }
> > 
> > static int mptcp_pm_set_flags(struct genl_info *info)
> > {
> >         struct mptcp_pm_addr_entry loc = { .addr = { .family =
> > AF_UNSPEC }, };
> >         struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
> >         struct nlattr *attr_loc, *attr_rem;
> >         int ret;
> > 
> >         if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
> >                 return -EINVAL;
> > 
> >         attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
> >         ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
> >         if (ret < 0)
> >                 return ret;
> > 
> >         if (info->attrs[MPTCP_PM_ATTR_TOKEN]) {
> >                 if (GENL_REQ_ATTR_CHECK(info,
> > MPTCP_PM_ATTR_ADDR_REMOTE))
> >                         return -EINVAL;
> > 
> >                 attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
> >                 ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
> >                 if (ret < 0)
> >                         return ret;
> > 
> >                 if (rem.family == AF_UNSPEC) {
> >                         NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
> >                                             "invalid remote address
> > family");
> >                         return -EINVAL;
> >                 }
> > 
> >                 return mptcp_userspace_pm_set_flags(&loc, &rem,
> > info);
> >         }
> 
> The problem is the same: ↑ is specific to the userspace PM, why
> moving
> the code here in the common section then?
> 
> Same for the code ↓.
> 
> So at the end, the only common code is the parsing of the local
> address,
> so just GENL_REQ_ATTR_CHECK(MPTCP_PM_ATTR_ADDR) and
> mptcp_pm_parse_entry(MPTCP_PM_ATTR_ADDR). Is it worth it?
> 
> So if we want to share code, all we can get I think is this:
> 
>   int mptcp_pm_nl_set_flags_doit(...)
>   {
>       (...)
> 
>       if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
>           return -EINVAL;
> 
>       attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
>       ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
>       if (ret < 0)
>           return ret;
> 
>       if (info->attrs[MPTCP_PM_ATTR_TOKEN])
>            return mptcp_userspace_pm_set_flags(&loc, info);
> 
>       return mptcp_pm_nl_set_flags(&loc, info);
>   }
> 
> Not a lot to share, but at least there is nothing PM specific here,
> except to pick the interface to continue with. And yes, that's
> something
> that could be done, but that's not much...
> 
> > 
> >         if (loc.addr.family == AF_UNSPEC) {
> >                 if (!loc.addr.id) {
> >                         NL_SET_ERR_MSG_ATTR(info->extack, attr_loc,
> >                                             "missing address ID");
> >                         return -EOPNOTSUPP;
> > 		}
> >         }
> > 
> >         return mptcp_pm_nl_set_flags(&loc, info);
> > }
> > 
> > WDYT?
> > 
> > -Geliang
> > 
> > > 
> > > Cheers,
> > > Matt
> > 
> 
> Cheers,
> Matt