On Tue, May 7, 2024 at 9:03 AM Matthieu Baerts <matttbe@xxxxxxxxxx> wrote:
>
> Hi Alexei,
>
> Thank you for the review!
>
> On 07/05/2024 16:49, Alexei Starovoitov wrote:
> > On Tue, May 7, 2024 at 3:53 AM Matthieu Baerts (NGI0)
> > <matttbe@xxxxxxxxxx> wrote:
> >>
> >> From: Nicolas Rybowski <nicolas.rybowski@xxxxxxxxxxxx>
> >>
> >> Move Nicolas's patch into bpf selftests directory. This example added a
> >> test that was adding a different mark (SO_MARK) on each subflow, and
> >> changing the TCP CC only on the first subflow.
> >>
> >> This example shows how it is possible to:
> >>
> >> Identify the parent msk of an MPTCP subflow.
> >> Put different sockopt for each subflow of a same MPTCP connection.
> >>
> >> Here especially, we implemented two different behaviours:
> >>
> >> A socket mark (SOL_SOCKET SO_MARK) is put on each subflow of a same
> >> MPTCP connection. The order of creation of the current subflow defines
> >> its mark.
> >
> >> The TCP CC algorithm of the very first subflow of an MPTCP
> >> connection is set to "reno".
> >
> > why?
> > What does it test?
> > That bpf_setsockopt() can actually do it?
>
> Correct.
>
> Here is a bit of context: from the userspace, an application can do a
> setsockopt() on an MPTCP socket, and typically the same value will be
> set on all subflows (paths). If someone wants to have different values
> per subflow, the recommanded way is to use BPF.
>
> We can indeed restrict this test to changing the MARK only. I think the
> CC has been modified just not to check one thing, but also to change
> something at the TCP level, because it is managed differently on MPTCP
> side -- but only when the userspace set something, or when new subflows
> are created. The result of this operation is easy to check with 'ss',
> and it was to show an exemple where this is set only on one subflow.
>
> > But the next patch doesn't check that it's reno.
>
> No, I think it is checked: 'reno' is not hardcoded, but 'skel->data->cc'
> is used instead:
>
> run_subflow(skel->data->cc);
>
> > It looks to me that dropping this "set to reno" part
> > won't change the purpose of the rest of selftest.
>
> Yes, up to you. If you still think it is better without it, we can
> remove the modification of the CC in patch 3/4, and the validation in
> patch 4/4.
The concern with picking reno is extra deps to CI and every developer.
Currently in selftests/bpf/config we do:
CONFIG_TCP_CONG_DCTCP=y
CONFIG_TCP_CONG_BBR=y
I'd like to avoid adding reno there as well.
Will bpf_setsockopt("dctcp") work?
