Re: [PATCH mptcp-next v9 0/7] add mptcp_subflow bpf_iter

[Geliang Tang <geliang@kernel.org>]

On Tue, 2024-10-15 at 11:01 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your reply!
> 
> On 15/10/2024 09:27, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for this review.
> > 
> > On Mon, 2024-10-14 at 18:08 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 09/10/2024 12:05, MPTCP CI wrote:
> > > > Hi Geliang,
> > > > 
> > > > Thank you for your modifications, that's great!
> > > > 
> > > > But sadly, our CI spotted some issues with it when trying to
> > > > build
> > > > it.
> > > > 
> > > > You can find more details there:
> > > > 
> > > >  
> > > > https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11252652867
> > > 
> > > I was looking at applying this series, but there are some issues
> > > reported by the CI:
> > > 
> > >   warning: symbol 'bpf_*mptcp_*' was not declared. Should it be
> > > static?
> > > 
> > > Could it be possible to have a fix for that please before
> > > applying
> > > the
> > > series?
> > 
> > No fix is ​​needed, just ignore these warnings.
> 
> If possible, I would prefer not to ignore these warnings, because
> other
> CI might report the same issue. I didn't check, but can you not
> simply
> declare these new helpers as "static"? It looks like we can have
> kfunc
> declared as static, no?
> 
> > This error is also
> > reported in other places:
> > 
> > $ make C=1 -o net/socket.o
> >   CALL    scripts/checksyscalls.sh
> >   DESCEND objtool
> >   INSTALL libsubcmd_headers
> >   DESCEND bpf/resolve_btfids
> >   INSTALL libsubcmd_headers
> >   CC      net/socket.o
> >   CHECK   net/socket.c
> > net/socket.c:1704:21: warning: symbol 'update_socket_protocol' was
> > not
> > declared. Should it be static?
> 
> In this example, you are showing one symbol that has been added for
> MPTCP, maybe we forgot something :)
> 
> > It seems that it is because "-Wmissing-declarations" is not
> > recognized
> > by sparse
> 
> I don't see complains about that when introducing new kfunc, maybe we
> are supposed to do something else to avoid that?
> 
> > > I guess you are missing __bpf_kfunc_start_defs() and
> > > __bpf_kfunc_end_defs() around the declaration of the BPF
> > > dedicated
> > > kfunc, no?
> > 
> > No. __bpf_kfunc_start_defs() and __bpf_kfunc_end_defs() are indeed
> > used
> > in patch 2.
> 
> Thanks, I missed that.
> 
> > > Also, where should I apply these patches? Before "mptcp: add
> > > sched_data
> > > helpers"?
> > 
> > Yes, before "mptcp: add sched_data helpers", after "selftests/bpf:
> > Add
> > mptcp subflow subtest".
> 
> OK!
> 
> > > But then there should not be any dependences on the BPF
> > > scheduler work (and I think that would be better without this
> > > dependence, see my comment on patch 4/7)
> > 
> > This set doesn't have any dependence on the BPF packet scheduler
> > since
> > the selftest is added as a ftrace. It somehow depends on packet
> > scheduler since it invoke some packet scheduler functions such as
> > mptcp_subflow_active() and bpf_mptcp_subflow_tcp_sock().
> 
> OK, but if I insert the series just after "selftests/bpf: Add mptcp
> subflow subtest", it will not have access to mptcp_subflow_active().
> I
> still need to check your reply on the patch 4/7, but it would be
> easier
> if the test doesn't depend on "mptcp_subflow_active()": it's just a

Do you mean only drop "mptcp_subflow_active" and
"mptcp_subflow_set_scheduled" in the test or drop all the helpers,
include "bpf_mptcp_subflow_tcp_sock" and "bpf_mptcp_subflow_ctx"?

If it is the latter, the test is simplified like this, right?

        if (bpf_get_current_pid_tgid() >> 32 != pid)
                return 0;

        if (msk->pm.server_side || !msk->pm.subflows)
                return 0;

        ids = 0;

        msk = bpf_mptcp_sock_acquire(msk);
        if (!msk)
                return 0;
        bpf_for_each(mptcp_subflow, subflow, msk) {
                ids += subflow->subflow_id;
        bpf_mptcp_sock_release(msk);

        return 0;

To me both are fine, which one do you prefer? I can refactor the code
and send the squash-to patches. Or maybe a v10 is better?

Thanks,
-Geliang

> test
> in preparation to the packet scheduler, it is fine if it is not doing
> interesting for the moment I think.
> 
> Cheers,
> Matt