[bpf-next,V2] bpf: make list_for_each_entry portable

Commit Message

Jose E. Marchesi May 11, 2024, 9:22 p.m. UTC

[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 <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: Alexei Starovoitov <[e-mail]>
---
 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(-)

Comments

Alexei Starovoitov May 13, 2024, 12:46 a.m. UTC | #1

On Sat, May 11, 2024 at 2:23 PM Jose E. Marchesi
<[e-mail]> 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 <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Alexei Starovoitov <[e-mail]>
> ---
>  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!

[e-mail] May 13, 2024, 12:50 a.m. UTC | #2

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[e-mail]>:

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!

Patch

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;