1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v2 0/5] Squash to "Add mptcp_subflow bpf_iter support"
  4. View patch

[PATCH mptcp-next v2 5/5] Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"

Geliang Tang posted 5 patches 3 months, 1 week ago
There is a newer version of this series
[PATCH mptcp-next v2 5/5] Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"
Posted by Geliang Tang 3 months, 1 week ago 
From: Geliang Tang <tanggeliang@kylinos.cn>

Use bpf_core_cast() instead of bpf_skc_to_mptcp_sock().
Change the 2nd parameter type of bpf_for_each() as 'struct sock'.
Drop bpf_mptcp_sock_acquire/release.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/bpf_experimental.h      | 2 +-
 tools/testing/selftests/bpf/progs/mptcp_bpf.h       | 3 ---
 tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 8 ++------
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 2ab3f0063c0f..6a96c56f0725 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -577,7 +577,7 @@ extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
 
 struct bpf_iter_mptcp_subflow;
 extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
-				      struct mptcp_sock *msk) __weak __ksym;
+				      struct sock *sk) __weak __ksym;
 extern struct mptcp_subflow_context *
 bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
 extern void
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index b1f6e1fb467e..ede9111ee597 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -43,9 +43,6 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
 }
 
 /* ksym */
-extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym;
-extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
-
 extern struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
 extern struct sock *
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
index fd5691a4073b..2c0d11cb9864 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
@@ -24,14 +24,11 @@ int iters_subflow(struct bpf_sockopt *ctx)
 	if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
 		return 1;
 
-	msk = bpf_skc_to_mptcp_sock(sk);
+	msk = bpf_core_cast(sk, struct mptcp_sock);
 	if (!msk || msk->pm.server_side || !msk->pm.subflows)
 		return 1;
 
-	msk = bpf_mptcp_sock_acquire(msk);
-	if (!msk)
-		return 1;
-	bpf_for_each(mptcp_subflow, subflow, msk) {
+	bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
 		/* Here MPTCP-specific packet scheduler kfunc can be called:
 		 * this test is not doing anything really useful, only to
 		 * verify the iteration works.
@@ -58,6 +55,5 @@ int iters_subflow(struct bpf_sockopt *ctx)
 	ids = local_ids;
 
 out:
-	bpf_mptcp_sock_release(msk);
 	return 1;
 }
-- 
2.43.0
Re: [PATCH mptcp-next v2 5/5] Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"
Posted by Mat Martineau 2 months, 4 weeks ago 
On Sun, 2 Feb 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Use bpf_core_cast() instead of bpf_skc_to_mptcp_sock().
> Change the 2nd parameter type of bpf_for_each() as 'struct sock'.
> Drop bpf_mptcp_sock_acquire/release.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> tools/testing/selftests/bpf/bpf_experimental.h      | 2 +-
> tools/testing/selftests/bpf/progs/mptcp_bpf.h       | 3 ---
> tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 8 ++------
> 3 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> index 2ab3f0063c0f..6a96c56f0725 100644
> --- a/tools/testing/selftests/bpf/bpf_experimental.h
> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> @@ -577,7 +577,7 @@ extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
>
> struct bpf_iter_mptcp_subflow;
> extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
> -				      struct mptcp_sock *msk) __weak __ksym;
> +				      struct sock *sk) __weak __ksym;
> extern struct mptcp_subflow_context *
> bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
> extern void
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> index b1f6e1fb467e..ede9111ee597 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> @@ -43,9 +43,6 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
> }
>
> /* ksym */
> -extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym;
> -extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
> -
> extern struct mptcp_subflow_context *
> bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
> extern struct sock *
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
> index fd5691a4073b..2c0d11cb9864 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
> @@ -24,14 +24,11 @@ int iters_subflow(struct bpf_sockopt *ctx)
> 	if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
> 		return 1;
>
> -	msk = bpf_skc_to_mptcp_sock(sk);
> +	msk = bpf_core_cast(sk, struct mptcp_sock);
> 	if (!msk || msk->pm.server_side || !msk->pm.subflows)
> 		return 1;
>
> -	msk = bpf_mptcp_sock_acquire(msk);

Hi Geliang -

Why is the locking removed? The squashed commit will still have a commit 
message referring to the acquire/release, so that isn't consistent with 
the modified code.

bpf_iter_mptcp_subflow_new() does have the msk_owned_by_me() check, but on 
a kernel without CONFIG_LOCKDEP it doesn't do anything.

- Mat

> -	if (!msk)
> -		return 1;
> -	bpf_for_each(mptcp_subflow, subflow, msk) {
> +	bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
> 		/* Here MPTCP-specific packet scheduler kfunc can be called:
> 		 * this test is not doing anything really useful, only to
> 		 * verify the iteration works.
> @@ -58,6 +55,5 @@ int iters_subflow(struct bpf_sockopt *ctx)
> 	ids = local_ids;
>
> out:
> -	bpf_mptcp_sock_release(msk);
> 	return 1;
> }
> -- 
> 2.43.0
>
>
>
Re: [PATCH mptcp-next v2 5/5] Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"
Posted by Matthieu Baerts 2 months, 3 weeks ago 
Hi Mat, Geliang,

On 14/02/2025 02:45, Mat Martineau wrote:
> On Sun, 2 Feb 2025, Geliang Tang wrote:
> 
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> Use bpf_core_cast() instead of bpf_skc_to_mptcp_sock().
>> Change the 2nd parameter type of bpf_for_each() as 'struct sock'.
>> Drop bpf_mptcp_sock_acquire/release.
>>
>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>> ---
>> tools/testing/selftests/bpf/bpf_experimental.h      | 2 +-
>> tools/testing/selftests/bpf/progs/mptcp_bpf.h       | 3 ---
>> tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 8 ++------
>> 3 files changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/
>> testing/selftests/bpf/bpf_experimental.h
>> index 2ab3f0063c0f..6a96c56f0725 100644
>> --- a/tools/testing/selftests/bpf/bpf_experimental.h
>> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
>> @@ -577,7 +577,7 @@ extern void bpf_iter_css_destroy(struct
>> bpf_iter_css *it) __weak __ksym;
>>
>> struct bpf_iter_mptcp_subflow;
>> extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
>> -                      struct mptcp_sock *msk) __weak __ksym;
>> +                      struct sock *sk) __weak __ksym;
>> extern struct mptcp_subflow_context *
>> bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak
>> __ksym;
>> extern void
>> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/
>> testing/selftests/bpf/progs/mptcp_bpf.h
>> index b1f6e1fb467e..ede9111ee597 100644
>> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
>> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
>> @@ -43,9 +43,6 @@ mptcp_subflow_tcp_sock(const struct
>> mptcp_subflow_context *subflow)
>> }
>>
>> /* ksym */
>> -extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock
>> *msk) __ksym;
>> -extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
>> -
>> extern struct mptcp_subflow_context *
>> bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
>> extern struct sock *
>> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/
>> tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
>> index fd5691a4073b..2c0d11cb9864 100644
>> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
>> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
>> @@ -24,14 +24,11 @@ int iters_subflow(struct bpf_sockopt *ctx)
>>     if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
>>         return 1;
>>
>> -    msk = bpf_skc_to_mptcp_sock(sk);
>> +    msk = bpf_core_cast(sk, struct mptcp_sock);
>>     if (!msk || msk->pm.server_side || !msk->pm.subflows)
>>         return 1;
>>
>> -    msk = bpf_mptcp_sock_acquire(msk);
> 
> Hi Geliang -
> 
> Why is the locking removed? The squashed commit will still have a commit
> message referring to the acquire/release, so that isn't consistent with
> the modified code.

From what I understood from Martin's review [1], this mptcp_sock_acquire
helper was a workaround only to please the verifier, but they were not
needed

@Geliang: is this correct? (if yes, do not hesitate to add such info in
the commit message removing such helpers because without the context,
"Drop this patch as Martin suggested" is hard to understand :) )

1 https://lore.kernel.org/9b373a23-c093-42d8-b4ae-99f2e62e7681@linux.dev

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.