diff mbox series

[v2,bpf-next,12/14] selftests/bpf: convert pyperf, strobemeta, and l4lb_noinline to __noinline

Message ID 20200901015003.2871861-13-andriin@fb.com
State New
Headers show
Series [v2,bpf-next,01/14] libbpf: ensure ELF symbols table is found before further ELF processing | expand

Commit Message

Andrii Nakryiko Sept. 1, 2020, 1:50 a.m. UTC 
Convert few bigger selftests utilizing helper functions from __always_inline
to __noinline to excercise libbpf's bpf2bpf handling logic. Also split
l4lb_all selftest into two sub-tests.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 .../selftests/bpf/prog_tests/l4lb_all.c       |  9 ++--
 tools/testing/selftests/bpf/progs/pyperf.h    | 10 ++---
 .../testing/selftests/bpf/progs/strobemeta.h  | 15 ++++---
 .../selftests/bpf/progs/test_l4lb_noinline.c  | 41 +++++++++----------
 4 files changed, 34 insertions(+), 41 deletions(-)

Comments

Alexei Starovoitov Sept. 2, 2020, 5:45 a.m. UTC | #1 
On Mon, Aug 31, 2020 at 06:50:01PM -0700, Andrii Nakryiko wrote:
> diff --git a/tools/testing/selftests/bpf/progs/pyperf.h b/tools/testing/selftests/bpf/progs/pyperf.h
> index cc615b82b56e..13998aee887f 100644
> --- a/tools/testing/selftests/bpf/progs/pyperf.h
> +++ b/tools/testing/selftests/bpf/progs/pyperf.h
> @@ -67,7 +67,7 @@ typedef struct {
>  	void* co_name; // PyCodeObject.co_name
>  } FrameData;
>  
> -static __always_inline void *get_thread_state(void *tls_base, PidData *pidData)
> +static __noinline void *get_thread_state(void *tls_base, PidData *pidData)
>  {
>  	void* thread_state;
>  	int key;
> @@ -154,12 +154,10 @@ struct {
>  	__uint(value_size, sizeof(long long) * 127);
>  } stackmap SEC(".maps");
>  
> -#ifdef GLOBAL_FUNC
> -__attribute__((noinline))
> -#else
> -static __always_inline
> +#ifndef GLOBAL_FUNC
> +static
>  #endif
> -int __on_event(struct bpf_raw_tracepoint_args *ctx)
> +__noinline int __on_event(struct bpf_raw_tracepoint_args *ctx)
>  {
>  	uint64_t pid_tgid = bpf_get_current_pid_tgid();
>  	pid_t pid = (pid_t)(pid_tgid >> 32);
> diff --git a/tools/testing/selftests/bpf/progs/strobemeta.h b/tools/testing/selftests/bpf/progs/strobemeta.h
> index ad61b722a9de..d307c67ce52e 100644
> --- a/tools/testing/selftests/bpf/progs/strobemeta.h
> +++ b/tools/testing/selftests/bpf/progs/strobemeta.h
> @@ -266,8 +266,7 @@ struct tls_index {
>  	uint64_t offset;
>  };
>  
> -static __always_inline void *calc_location(struct strobe_value_loc *loc,
> -					   void *tls_base)
> +static __noinline void *calc_location(struct strobe_value_loc *loc, void *tls_base)

hmm. this reduces the existing test coverage. Unless I'm misreading it.
Could you keep existing strobemta tests and add new one?
With new ifdefs. Like this GLOBAL_FUNC.
Andrii Nakryiko Sept. 2, 2020, 7:58 p.m. UTC | #2 
On Tue, Sep 1, 2020 at 10:45 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Aug 31, 2020 at 06:50:01PM -0700, Andrii Nakryiko wrote:
> > diff --git a/tools/testing/selftests/bpf/progs/pyperf.h b/tools/testing/selftests/bpf/progs/pyperf.h
> > index cc615b82b56e..13998aee887f 100644
> > --- a/tools/testing/selftests/bpf/progs/pyperf.h
> > +++ b/tools/testing/selftests/bpf/progs/pyperf.h
> > @@ -67,7 +67,7 @@ typedef struct {
> >       void* co_name; // PyCodeObject.co_name
> >  } FrameData;
> >
> > -static __always_inline void *get_thread_state(void *tls_base, PidData *pidData)
> > +static __noinline void *get_thread_state(void *tls_base, PidData *pidData)
> >  {
> >       void* thread_state;
> >       int key;
> > @@ -154,12 +154,10 @@ struct {
> >       __uint(value_size, sizeof(long long) * 127);
> >  } stackmap SEC(".maps");
> >
> > -#ifdef GLOBAL_FUNC
> > -__attribute__((noinline))
> > -#else
> > -static __always_inline
> > +#ifndef GLOBAL_FUNC
> > +static
> >  #endif
> > -int __on_event(struct bpf_raw_tracepoint_args *ctx)
> > +__noinline int __on_event(struct bpf_raw_tracepoint_args *ctx)
> >  {
> >       uint64_t pid_tgid = bpf_get_current_pid_tgid();
> >       pid_t pid = (pid_t)(pid_tgid >> 32);
> > diff --git a/tools/testing/selftests/bpf/progs/strobemeta.h b/tools/testing/selftests/bpf/progs/strobemeta.h
> > index ad61b722a9de..d307c67ce52e 100644
> > --- a/tools/testing/selftests/bpf/progs/strobemeta.h
> > +++ b/tools/testing/selftests/bpf/progs/strobemeta.h
> > @@ -266,8 +266,7 @@ struct tls_index {
> >       uint64_t offset;
> >  };
> >
> > -static __always_inline void *calc_location(struct strobe_value_loc *loc,
> > -                                        void *tls_base)
> > +static __noinline void *calc_location(struct strobe_value_loc *loc, void *tls_base)
>
> hmm. this reduces the existing test coverage. Unless I'm misreading it.
> Could you keep existing strobemta tests and add new one?
> With new ifdefs. Like this GLOBAL_FUNC.

Oh, you mean testing single BPF program complexity when everything is
inlined? Yeah, haven't thought about that. Ok, I'll add new variants
with or without subprogram calls.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
index c2d373e294bb..8073105548ff 100644
--- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
+++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
@@ -80,9 +80,8 @@  static void test_l4lb(const char *file)
 
 void test_l4lb_all(void)
 {
-	const char *file1 = "./test_l4lb.o";
-	const char *file2 = "./test_l4lb_noinline.o";
-
-	test_l4lb(file1);
-	test_l4lb(file2);
+	if (test__start_subtest("l4lb_inline"))
+		test_l4lb("test_l4lb.o");
+	if (test__start_subtest("l4lb_noinline"))
+		test_l4lb("test_l4lb_noinline.o");
 }
diff --git a/tools/testing/selftests/bpf/progs/pyperf.h b/tools/testing/selftests/bpf/progs/pyperf.h
index cc615b82b56e..13998aee887f 100644
--- a/tools/testing/selftests/bpf/progs/pyperf.h
+++ b/tools/testing/selftests/bpf/progs/pyperf.h
@@ -67,7 +67,7 @@  typedef struct {
 	void* co_name; // PyCodeObject.co_name
 } FrameData;
 
-static __always_inline void *get_thread_state(void *tls_base, PidData *pidData)
+static __noinline void *get_thread_state(void *tls_base, PidData *pidData)
 {
 	void* thread_state;
 	int key;
@@ -154,12 +154,10 @@  struct {
 	__uint(value_size, sizeof(long long) * 127);
 } stackmap SEC(".maps");
 
-#ifdef GLOBAL_FUNC
-__attribute__((noinline))
-#else
-static __always_inline
+#ifndef GLOBAL_FUNC
+static
 #endif
-int __on_event(struct bpf_raw_tracepoint_args *ctx)
+__noinline int __on_event(struct bpf_raw_tracepoint_args *ctx)
 {
 	uint64_t pid_tgid = bpf_get_current_pid_tgid();
 	pid_t pid = (pid_t)(pid_tgid >> 32);
diff --git a/tools/testing/selftests/bpf/progs/strobemeta.h b/tools/testing/selftests/bpf/progs/strobemeta.h
index ad61b722a9de..d307c67ce52e 100644
--- a/tools/testing/selftests/bpf/progs/strobemeta.h
+++ b/tools/testing/selftests/bpf/progs/strobemeta.h
@@ -266,8 +266,7 @@  struct tls_index {
 	uint64_t offset;
 };
 
-static __always_inline void *calc_location(struct strobe_value_loc *loc,
-					   void *tls_base)
+static __noinline void *calc_location(struct strobe_value_loc *loc, void *tls_base)
 {
 	/*
 	 * tls_mode value is:
@@ -327,10 +326,10 @@  static __always_inline void *calc_location(struct strobe_value_loc *loc,
 		: NULL;
 }
 
-static __always_inline void read_int_var(struct strobemeta_cfg *cfg,
-					 size_t idx, void *tls_base,
-					 struct strobe_value_generic *value,
-					 struct strobemeta_payload *data)
+static __noinline void read_int_var(struct strobemeta_cfg *cfg,
+				    size_t idx, void *tls_base,
+				    struct strobe_value_generic *value,
+				    struct strobemeta_payload *data)
 {
 	void *location = calc_location(&cfg->int_locs[idx], tls_base);
 	if (!location)
@@ -440,8 +439,8 @@  static __always_inline void *read_map_var(struct strobemeta_cfg *cfg,
  * read_strobe_meta returns NULL, if no metadata was read; otherwise returns
  * pointer to *right after* payload ends
  */
-static __always_inline void *read_strobe_meta(struct task_struct *task,
-					      struct strobemeta_payload *data)
+static __noinline void *read_strobe_meta(struct task_struct *task,
+					 struct strobemeta_payload *data)
 {
 	pid_t pid = bpf_get_current_pid_tgid() >> 32;
 	struct strobe_value_generic value = {0};
diff --git a/tools/testing/selftests/bpf/progs/test_l4lb_noinline.c b/tools/testing/selftests/bpf/progs/test_l4lb_noinline.c
index 28351936a438..b9e2753f4f91 100644
--- a/tools/testing/selftests/bpf/progs/test_l4lb_noinline.c
+++ b/tools/testing/selftests/bpf/progs/test_l4lb_noinline.c
@@ -17,9 +17,7 @@ 
 #include "test_iptunnel_common.h"
 #include <bpf/bpf_endian.h>
 
-int _version SEC("version") = 1;
-
-static __u32 rol32(__u32 word, unsigned int shift)
+static __always_inline __u32 rol32(__u32 word, unsigned int shift)
 {
 	return (word << shift) | (word >> ((-shift) & 31));
 }
@@ -52,7 +50,7 @@  static __u32 rol32(__u32 word, unsigned int shift)
 
 typedef unsigned int u32;
 
-static u32 jhash(const void *key, u32 length, u32 initval)
+static __noinline u32 jhash(const void *key, u32 length, u32 initval)
 {
 	u32 a, b, c;
 	const unsigned char *k = key;
@@ -88,7 +86,7 @@  static u32 jhash(const void *key, u32 length, u32 initval)
 	return c;
 }
 
-static u32 __jhash_nwords(u32 a, u32 b, u32 c, u32 initval)
+static __noinline u32 __jhash_nwords(u32 a, u32 b, u32 c, u32 initval)
 {
 	a += initval;
 	b += initval;
@@ -97,7 +95,7 @@  static u32 __jhash_nwords(u32 a, u32 b, u32 c, u32 initval)
 	return c;
 }
 
-static u32 jhash_2words(u32 a, u32 b, u32 initval)
+static __noinline u32 jhash_2words(u32 a, u32 b, u32 initval)
 {
 	return __jhash_nwords(a, b, 0, initval + JHASH_INITVAL + (2 << 2));
 }
@@ -200,8 +198,7 @@  struct {
 	__type(value, struct ctl_value);
 } ctl_array SEC(".maps");
 
-static __u32 get_packet_hash(struct packet_description *pckt,
-			     bool ipv6)
+static __noinline __u32 get_packet_hash(struct packet_description *pckt, bool ipv6)
 {
 	if (ipv6)
 		return jhash_2words(jhash(pckt->srcv6, 16, MAX_VIPS),
@@ -210,10 +207,10 @@  static __u32 get_packet_hash(struct packet_description *pckt,
 		return jhash_2words(pckt->src, pckt->ports, CH_RINGS_SIZE);
 }
 
-static bool get_packet_dst(struct real_definition **real,
-			   struct packet_description *pckt,
-			   struct vip_meta *vip_info,
-			   bool is_ipv6)
+static __noinline bool get_packet_dst(struct real_definition **real,
+				      struct packet_description *pckt,
+				      struct vip_meta *vip_info,
+				      bool is_ipv6)
 {
 	__u32 hash = get_packet_hash(pckt, is_ipv6);
 	__u32 key = RING_SIZE * vip_info->vip_num + hash % RING_SIZE;
@@ -233,8 +230,8 @@  static bool get_packet_dst(struct real_definition **real,
 	return true;
 }
 
-static int parse_icmpv6(void *data, void *data_end, __u64 off,
-			struct packet_description *pckt)
+static __noinline int parse_icmpv6(void *data, void *data_end, __u64 off,
+				   struct packet_description *pckt)
 {
 	struct icmp6hdr *icmp_hdr;
 	struct ipv6hdr *ip6h;
@@ -255,8 +252,8 @@  static int parse_icmpv6(void *data, void *data_end, __u64 off,
 	return TC_ACT_UNSPEC;
 }
 
-static int parse_icmp(void *data, void *data_end, __u64 off,
-		      struct packet_description *pckt)
+static __noinline int parse_icmp(void *data, void *data_end, __u64 off,
+				 struct packet_description *pckt)
 {
 	struct icmphdr *icmp_hdr;
 	struct iphdr *iph;
@@ -280,8 +277,8 @@  static int parse_icmp(void *data, void *data_end, __u64 off,
 	return TC_ACT_UNSPEC;
 }
 
-static bool parse_udp(void *data, __u64 off, void *data_end,
-		      struct packet_description *pckt)
+static __noinline bool parse_udp(void *data, __u64 off, void *data_end,
+				 struct packet_description *pckt)
 {
 	struct udphdr *udp;
 	udp = data + off;
@@ -299,8 +296,8 @@  static bool parse_udp(void *data, __u64 off, void *data_end,
 	return true;
 }
 
-static bool parse_tcp(void *data, __u64 off, void *data_end,
-		      struct packet_description *pckt)
+static __noinline bool parse_tcp(void *data, __u64 off, void *data_end,
+				 struct packet_description *pckt)
 {
 	struct tcphdr *tcp;
 
@@ -321,8 +318,8 @@  static bool parse_tcp(void *data, __u64 off, void *data_end,
 	return true;
 }
 
-static int process_packet(void *data, __u64 off, void *data_end,
-			  bool is_ipv6, struct __sk_buff *skb)
+static __noinline int process_packet(void *data, __u64 off, void *data_end,
+				     bool is_ipv6, struct __sk_buff *skb)
 {
 	void *pkt_start = (void *)(long)skb->data;
 	struct packet_description pckt = {};