* [PATCH bpf-next V2] bpf: make list_for_each_entry portable
@ 2024-05-11 21:22 Jose E. Marchesi
2024-05-13 0:46 ` Alexei Starovoitov
2024-05-13 0:50 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Jose E. Marchesi @ 2024-05-11 21:22 UTC (permalink / raw)
To: bpf; +Cc: Jose E . Marchesi, david.faust, cupertino.miranda,
Alexei Starovoitov
[Changes from V1:
- The __compat_break has been abandoned in favor of
a more readable can_loop macro that can be used anywhere, including
loop conditions.]
The macro list_for_each_entry is defined in bpf_arena_list.h as
follows:
#define list_for_each_entry(pos, head, member) \
for (void * ___tmp = (pos = list_entry_safe((head)->first, \
typeof(*(pos)), member), \
(void *)0); \
pos && ({ ___tmp = (void *)pos->member.next; 1; }); \
cond_break, \
pos = list_entry_safe((void __arena *)___tmp, typeof(*(pos)), member))
The macro cond_break, in turn, expands to a statement expression that
contains a `break' statement. Compound statement expressions, and the
subsequent ability of placing statements in the header of a `for'
loop, are GNU extensions.
Unfortunately, clang implements this GNU extension differently than
GCC:
- In GCC the `break' statement is bound to the containing "breakable"
context in which the defining `for' appears. If there is no such
context, GCC emits a warning: break statement without enclosing `for'
o `switch' statement.
- In clang the `break' statement is bound to the defining `for'. If
the defining `for' is itself inside some breakable construct, then
clang emits a -Wgcc-compat warning.
This patch adds a new macro can_loop to bpf_experimental, that
implements the same logic than cond_break but evaluates to a boolean
expression. The patch also changes all the current instances of usage
of cond_break withing the header of loop accordingly.
Tested in bpf-next master.
No regressions.
Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
Cc: david.faust@oracle.com
Cc: cupertino.miranda@oracle.com
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
---
tools/testing/selftests/bpf/bpf_arena_list.h | 4 +--
.../testing/selftests/bpf/bpf_experimental.h | 28 +++++++++++++++++++
.../testing/selftests/bpf/progs/arena_list.c | 2 +-
.../bpf/progs/verifier_iterating_callbacks.c | 9 +++---
4 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_arena_list.h b/tools/testing/selftests/bpf/bpf_arena_list.h
index b99b9f408eff..85dbc3ea4da5 100644
--- a/tools/testing/selftests/bpf/bpf_arena_list.h
+++ b/tools/testing/selftests/bpf/bpf_arena_list.h
@@ -29,6 +29,7 @@ static inline void *bpf_iter_num_new(struct bpf_iter_num *it, int i, int j) { re
static inline void bpf_iter_num_destroy(struct bpf_iter_num *it) {}
static inline bool bpf_iter_num_next(struct bpf_iter_num *it) { return true; }
#define cond_break ({})
+#define can_loop true
#endif
/* Safely walk link list elements. Deletion of elements is allowed. */
@@ -36,8 +37,7 @@ static inline bool bpf_iter_num_next(struct bpf_iter_num *it) { return true; }
for (void * ___tmp = (pos = list_entry_safe((head)->first, \
typeof(*(pos)), member), \
(void *)0); \
- pos && ({ ___tmp = (void *)pos->member.next; 1; }); \
- cond_break, \
+ pos && ({ ___tmp = (void *)pos->member.next; 1; }) && can_loop; \
pos = list_entry_safe((void __arena *)___tmp, typeof(*(pos)), member))
static inline void list_add_head(arena_list_node_t *n, arena_list_head_t *h)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 8b9cc87be4c4..13e79af0a17c 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -326,7 +326,21 @@ l_true: \
})
#endif
+/* Note that cond_break can only be portably used in the body of a
+ breakable construct, whereas can_loop can be used anywhere. */
+
#ifdef __BPF_FEATURE_MAY_GOTO
+#define can_loop \
+ ({ __label__ l_break, l_continue; \
+ bool ret = true; \
+ asm volatile goto("may_goto %l[l_break]" \
+ :::: l_break); \
+ goto l_continue; \
+ l_break: ret = false; \
+ l_continue:; \
+ ret; \
+ })
+
#define cond_break \
({ __label__ l_break, l_continue; \
asm volatile goto("may_goto %l[l_break]" \
@@ -336,6 +350,20 @@ l_true: \
l_continue:; \
})
#else
+#define can_loop \
+ ({ __label__ l_break, l_continue; \
+ bool ret = true; \
+ asm volatile goto("1:.byte 0xe5; \
+ .byte 0; \
+ .long ((%l[l_break] - 1b - 8) / 8) & 0xffff; \
+ .short 0" \
+ :::: l_break); \
+ goto l_continue; \
+ l_break: ret = false; \
+ l_continue:; \
+ ret; \
+ })
+
#define cond_break \
({ __label__ l_break, l_continue; \
asm volatile goto("1:.byte 0xe5; \
diff --git a/tools/testing/selftests/bpf/progs/arena_list.c b/tools/testing/selftests/bpf/progs/arena_list.c
index c0422c58cee2..93bd0600eba0 100644
--- a/tools/testing/selftests/bpf/progs/arena_list.c
+++ b/tools/testing/selftests/bpf/progs/arena_list.c
@@ -49,7 +49,7 @@ int arena_list_add(void *ctx)
list_head = &global_head;
- for (i = zero; i < cnt; cond_break, i++) {
+ for (i = zero; i < cnt && can_loop; i++) {
struct elem __arena *n = bpf_alloc(sizeof(*n));
test_val++;
diff --git a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
index 99e561f18f9b..bd676d7e615f 100644
--- a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
+++ b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
@@ -318,7 +318,7 @@ int cond_break1(const void *ctx)
unsigned long i;
unsigned int sum = 0;
- for (i = zero; i < ARR_SZ; cond_break, i++)
+ for (i = zero; i < ARR_SZ && can_loop; i++)
sum += i;
for (i = zero; i < ARR_SZ; i++) {
barrier_var(i);
@@ -336,12 +336,11 @@ int cond_break2(const void *ctx)
int i, j;
int sum = 0;
- for (i = zero; i < 1000; cond_break, i++)
+ for (i = zero; i < 1000 && can_loop; i++)
for (j = zero; j < 1000; j++) {
sum += i + j;
cond_break;
- }
-
+ }
return sum;
}
@@ -349,7 +348,7 @@ static __noinline int loop(void)
{
int i, sum = 0;
- for (i = zero; i <= 1000000; i++, cond_break)
+ for (i = zero; i <= 1000000 && can_loop; i++)
sum += i;
return sum;
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next V2] bpf: make list_for_each_entry portable
2024-05-11 21:22 [PATCH bpf-next V2] bpf: make list_for_each_entry portable Jose E. Marchesi
@ 2024-05-13 0:46 ` Alexei Starovoitov
2024-05-13 0:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2024-05-13 0:46 UTC (permalink / raw)
To: Jose E. Marchesi; +Cc: bpf, David Faust, Cupertino Miranda
On Sat, May 11, 2024 at 2:23 PM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
> [Changes from V1:
> - The __compat_break has been abandoned in favor of
> a more readable can_loop macro that can be used anywhere, including
> loop conditions.]
>
> The macro list_for_each_entry is defined in bpf_arena_list.h as
> follows:
>
> #define list_for_each_entry(pos, head, member) \
> for (void * ___tmp = (pos = list_entry_safe((head)->first, \
> typeof(*(pos)), member), \
> (void *)0); \
> pos && ({ ___tmp = (void *)pos->member.next; 1; }); \
> cond_break, \
> pos = list_entry_safe((void __arena *)___tmp, typeof(*(pos)), member))
>
> The macro cond_break, in turn, expands to a statement expression that
> contains a `break' statement. Compound statement expressions, and the
> subsequent ability of placing statements in the header of a `for'
> loop, are GNU extensions.
>
> Unfortunately, clang implements this GNU extension differently than
> GCC:
>
> - In GCC the `break' statement is bound to the containing "breakable"
> context in which the defining `for' appears. If there is no such
> context, GCC emits a warning: break statement without enclosing `for'
> o `switch' statement.
>
> - In clang the `break' statement is bound to the defining `for'. If
> the defining `for' is itself inside some breakable construct, then
> clang emits a -Wgcc-compat warning.
>
> This patch adds a new macro can_loop to bpf_experimental, that
> implements the same logic than cond_break but evaluates to a boolean
> expression. The patch also changes all the current instances of usage
> of cond_break withing the header of loop accordingly.
>
> Tested in bpf-next master.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> Cc: david.faust@oracle.com
> Cc: cupertino.miranda@oracle.com
> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> ---
> tools/testing/selftests/bpf/bpf_arena_list.h | 4 +--
> .../testing/selftests/bpf/bpf_experimental.h | 28 +++++++++++++++++++
> .../testing/selftests/bpf/progs/arena_list.c | 2 +-
> .../bpf/progs/verifier_iterating_callbacks.c | 9 +++---
> 4 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/bpf_arena_list.h b/tools/testing/selftests/bpf/bpf_arena_list.h
> index b99b9f408eff..85dbc3ea4da5 100644
> --- a/tools/testing/selftests/bpf/bpf_arena_list.h
> +++ b/tools/testing/selftests/bpf/bpf_arena_list.h
> @@ -29,6 +29,7 @@ static inline void *bpf_iter_num_new(struct bpf_iter_num *it, int i, int j) { re
> static inline void bpf_iter_num_destroy(struct bpf_iter_num *it) {}
> static inline bool bpf_iter_num_next(struct bpf_iter_num *it) { return true; }
> #define cond_break ({})
> +#define can_loop true
> #endif
>
> /* Safely walk link list elements. Deletion of elements is allowed. */
> @@ -36,8 +37,7 @@ static inline bool bpf_iter_num_next(struct bpf_iter_num *it) { return true; }
> for (void * ___tmp = (pos = list_entry_safe((head)->first, \
> typeof(*(pos)), member), \
> (void *)0); \
> - pos && ({ ___tmp = (void *)pos->member.next; 1; }); \
> - cond_break, \
> + pos && ({ ___tmp = (void *)pos->member.next; 1; }) && can_loop; \
> pos = list_entry_safe((void __arena *)___tmp, typeof(*(pos)), member))
>
> static inline void list_add_head(arena_list_node_t *n, arena_list_head_t *h)
> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> index 8b9cc87be4c4..13e79af0a17c 100644
> --- a/tools/testing/selftests/bpf/bpf_experimental.h
> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> @@ -326,7 +326,21 @@ l_true: \
> })
> #endif
>
> +/* Note that cond_break can only be portably used in the body of a
> + breakable construct, whereas can_loop can be used anywhere. */
I fixed this comment to be proper kernel style.
> +
> #ifdef __BPF_FEATURE_MAY_GOTO
> +#define can_loop \
> + ({ __label__ l_break, l_continue; \
fixed white space damage.
> + bool ret = true; \
> + asm volatile goto("may_goto %l[l_break]" \
fixed broken formatting.
> + :::: l_break); \
> + goto l_continue; \
> + l_break: ret = false; \
> + l_continue:; \
> + ret; \
> + })
> +
> #define cond_break \
> ({ __label__ l_break, l_continue; \
> asm volatile goto("may_goto %l[l_break]" \
> @@ -336,6 +350,20 @@ l_true: \
> l_continue:; \
> })
> #else
> +#define can_loop \
> + ({ __label__ l_break, l_continue; \
> + bool ret = true; \
> + asm volatile goto("1:.byte 0xe5; \
> + .byte 0; \
> + .long ((%l[l_break] - 1b - 8) / 8) & 0xffff; \
> + .short 0" \
> + :::: l_break); \
> + goto l_continue; \
> + l_break: ret = false; \
> + l_continue:; \
> + ret; \
> + })
> +
This copy paste of the macro is a bit annoying,
but I don't see a clean way to make can_loop and cond_break
to use a common macro without being unreadable.
So applied with the above fixes.
Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next V2] bpf: make list_for_each_entry portable
2024-05-11 21:22 [PATCH bpf-next V2] bpf: make list_for_each_entry portable Jose E. Marchesi
2024-05-13 0:46 ` Alexei Starovoitov
@ 2024-05-13 0:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-05-13 0:50 UTC (permalink / raw)
To: Jose E. Marchesi; +Cc: bpf, david.faust, cupertino.miranda, alexei.starovoitov
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Sat, 11 May 2024 23:22:43 +0200 you wrote:
> [Changes from V1:
> - The __compat_break has been abandoned in favor of
> a more readable can_loop macro that can be used anywhere, including
> loop conditions.]
>
> The macro list_for_each_entry is defined in bpf_arena_list.h as
> follows:
>
> [...]
Here is the summary with links:
- [bpf-next,V2] bpf: make list_for_each_entry portable
https://git.kernel.org/bpf/bpf-next/c/ba39486d2c43
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-13 0:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-11 21:22 [PATCH bpf-next V2] bpf: make list_for_each_entry portable Jose E. Marchesi
2024-05-13 0:46 ` Alexei Starovoitov
2024-05-13 0:50 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox