Re: [PATCH mptcp-next 2/2] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Geliang Tang <geliang@kernel.org>]

Hi Martin, Matt,

On Mon, 2024-08-26 at 22:22 -0700, Martin KaFai Lau wrote:
> On 8/26/24 2:24 AM, Geliang Tang wrote:
> > > I'm not an expert in this, but I guess it means you cannot use
> > > 'mptcp_subflow_active()', because it can modify the 'subflow'
> > > structure
> > > that you got with bpf_core_cast() for a read-only usage.
> > 
> > A read-only function will get the same error.
> > 
> > I added a read-only function mptcp_subflow_get_scheduled() for
> > testing:
> > 
> > bool mptcp_subflow_get_scheduled(struct mptcp_subflow_context
> > *subflow)
> > {
> >          return subflow->scheduled;
> > }
> > 
> > And invoke it from BPF in mptcp_for_each_subflow() loop:
> > 
> > int BPF_PROG(bpf_first_get_subflow, struct mptcp_sock *msk,
> >               struct mptcp_sched_data *data)
> > {
> >          struct mptcp_subflow_context *subflow, *tmp;
> > 
> >          mptcp_for_each_subflow(msk, tmp) {
> >                  subflow = bpf_core_cast(tmp, struct
> > mptcp_subflow_context);
> >                  mptcp_subflow_get_scheduled(subflow);
> >          }
> >          return 0;
> > }
> > 
> > The same "arg#0 is untrusted_ptr_ expected ptr_ or socket" occurs.
> > 
> > Hope Martin can give us a solution for this issue.
> 
> I don't know the context for this list walking + modify-by-kfunc
> usage, so the 
> following could be a grain of salt.
> 
> It seems like fitting the bpf_iter use case. Take a look at some
> recent bpf_iter 
> additions, e.g. bpf_iter_{task,css}_next(). Also the bpf_for_each
> macro usage in 
> selftests. There may be some secondary things that need to consider,
> e.g. how 
> the walked subflow is protected, rcu, refcnt...etc.

Great! Thanks. bpf_iter works well for MPTCP BPF scheduler.

I added a new bpf_iter type named "mptcp_subflow" in net/mptcp/bpf.c
like this:

'''
__bpf_kfunc_start_defs();

__bpf_kfunc int bpf_iter_mptcp_subflow_new(struct
bpf_iter_mptcp_subflow *it,
                struct mptcp_sock *msk, unsigned int flags)
{
        struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;

        kit->msk = msk;
        kit->pos = &msk->conn_list;
        spin_lock_bh(&msk->pm.lock);

        return 0;
}

__bpf_kfunc struct mptcp_subflow_context *
bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it)
{
        struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
        struct mptcp_subflow_context *subflow;
        struct mptcp_sock *msk = kit->msk;

        subflow = list_entry((kit->pos)->next, struct
mptcp_subflow_context, node);
        if (list_entry_is_head(subflow, &msk->conn_list, node))
                return NULL;

        kit->pos = &subflow->node;
        return subflow;
}

__bpf_kfunc void bpf_iter_mptcp_subflow_destroy(struct
bpf_iter_mptcp_subflow *it)
{
        struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
        struct mptcp_sock *msk = kit->msk;

        spin_unlock_bh(&msk->pm.lock);
}

__bpf_kfunc_end_defs();
'''

And use "bpf_for_each(mptcp_subflow)" like this in
progs/mptcp_bpf_burst.c:

'''
        i = 0;
        bpf_rcu_read_lock();
        bpf_for_each(mptcp_subflow, subflow, msk, 0) {
                bool backup = subflow->backup || subflow->request_bkup;

                if (i++ > MPTCP_SUBFLOWS_MAX)
                        break;

                ssk = mptcp_subflow_tcp_sock(subflow);
                if (!mptcp_subflow_active(subflow))
                        continue;

                nr_active += !backup;
                pace = subflow->avg_pacing_rate;
                if (!pace) {
                        /* init pacing rate from socket */
                        subflow->avg_pacing_rate = ssk->sk_pacing_rate;
                        pace = subflow->avg_pacing_rate;
                        if (!pace)
                                continue;
                }

                linger_time = div_u64((__u64)ssk->sk_wmem_queued << 32,
pace);
                if (linger_time < send_info[backup].linger_time) {
                        send_info[backup].subflow_id = i;
                        send_info[backup].linger_time = linger_time;
                }
        }
        bpf_rcu_read_unlock();
'''

With this mptcp_subflow bpf_iter, we can get rid of the subflows array
"contexts" in struct mptcp_sched_data:

'''
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index c3d0ea07cf0c..a739f917b054 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -104,8 +104,6 @@ struct mptcp_out_options {
 
 struct mptcp_sched_data {
        bool    reinject;
-       u8      subflows;
-       struct mptcp_subflow_context *contexts[MPTCP_SUBFLOWS_MAX];
 };
 
 struct mptcp_sched_ops {
'''

I'll send out these patches for review soon, with Martin's "Suggested-
by" tags.

Thanks,
-Geliang