[bpf-next] selftests: xsk: read current MAX_SKB_FRAGS from sysctl knob

Commit Message

Maciej Fijalkowski Sept. 9, 2024, 2:11 p.m. UTC

Currently, xskxceiver assumes that MAX_SKB_FRAGS value is always 17
which is not true - since the introduction of BIG TCP this can now take
any value between 17 to 45 via CONFIG_MAX_SKB_FRAGS.

Adjust the TOO_MANY_FRAGS test case to read the currently configured
MAX_SKB_FRAGS value by reading it from /proc/sys/net/core/max_skb_frags.

Signed-off-by: Maciej Fijalkowski <[e-mail]>
---
 tools/testing/selftests/bpf/xskxceiver.c | 41 +++++++++++++++++++++---
 tools/testing/selftests/bpf/xskxceiver.h |  1 -
 2 files changed, 36 insertions(+), 6 deletions(-)

Comments

Magnus Karlsson Sept. 10, 2024, 11:48 a.m. UTC | #1

On Mon, 9 Sept 2024 at 16:12, Maciej Fijalkowski
<[e-mail]> wrote:
>
> Currently, xskxceiver assumes that MAX_SKB_FRAGS value is always 17
> which is not true - since the introduction of BIG TCP this can now take
> any value between 17 to 45 via CONFIG_MAX_SKB_FRAGS.
>
> Adjust the TOO_MANY_FRAGS test case to read the currently configured
> MAX_SKB_FRAGS value by reading it from /proc/sys/net/core/max_skb_frags.
>
> Signed-off-by: Maciej Fijalkowski <[e-mail]>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 41 +++++++++++++++++++++---
>  tools/testing/selftests/bpf/xskxceiver.h |  1 -
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 92af633faea8..595b6da26897 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -325,6 +325,25 @@ static bool ifobj_zc_avail(struct ifobject *ifobject)
>         return zc_avail;
>  }
>
> +#define MAX_SKB_FRAGS_PATH "/proc/sys/net/core/max_skb_frags"
> +static unsigned int get_max_skb_frags(void)
> +{
> +       unsigned int max_skb_frags = 0;
> +       FILE *file;
> +
> +       file = fopen(MAX_SKB_FRAGS_PATH, "r");
> +       if (!file) {
> +               ksft_print_msg("Error opening %s\n", MAX_SKB_FRAGS_PATH);
> +               return 0;
> +       }
> +
> +       if (fscanf(file, "%u", &max_skb_frags) != 1)
> +               ksft_print_msg("Error reading %s\n", MAX_SKB_FRAGS_PATH);
> +
> +       fclose(file);
> +       return max_skb_frags;
> +}
> +
>  static struct option long_options[] = {
>         {"interface", required_argument, 0, 'i'},
>         {"busy-poll", no_argument, 0, 'b'},
> @@ -2245,13 +2264,22 @@ static int testapp_poll_rxq_tmout(struct test_spec *test)
>
>  static int testapp_too_many_frags(struct test_spec *test)
>  {
> -       struct pkt pkts[2 * XSK_DESC__MAX_SKB_FRAGS + 2] = {};
> +       struct pkt *pkts;
>         u32 max_frags, i;
> +       int ret;
>
> -       if (test->mode == TEST_MODE_ZC)
> +       if (test->mode == TEST_MODE_ZC) {
>                 max_frags = test->ifobj_tx->xdp_zc_max_segs;
> -       else
> -               max_frags = XSK_DESC__MAX_SKB_FRAGS;
> +       } else {
> +               max_frags = get_max_skb_frags();
> +               if (!max_frags)
> +                       return TEST_FAILURE;

Thanks for this fix Maciej. However, I think failing the test here is
a little bit too drastic. How about just returning TEST_SKIP and print
out that the max number of skbs is unknown as the reason for the skip?
Or even more optimistically, print out a warning that we could not
read the max number of skb but we are guessing 17 and then run the
test? If it passes, great we guessed correctly, but if it fails we are
not worse off than the current code. Do not know how often a file
system does not contain /proc/sys/net/core/max_skb_frags though.

> +               max_frags += 1;
> +       }
> +
> +       pkts = calloc(2 * max_frags + 2, sizeof(struct pkt));
> +       if (!pkts)
> +               return TEST_FAILURE;
>
>         test->mtu = MAX_ETH_JUMBO_SIZE;
>
> @@ -2281,7 +2309,10 @@ static int testapp_too_many_frags(struct test_spec *test)
>         pkts[2 * max_frags + 1].valid = true;
>
>         pkt_stream_generate_custom(test, pkts, 2 * max_frags + 2);
> -       return testapp_validate_traffic(test);
> +       ret = testapp_validate_traffic(test);
> +
> +       free(pkts);
> +       return ret;
>  }
>
>  static int xsk_load_xdp_programs(struct ifobject *ifobj)
> diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
> index 885c948c5d83..e46e823f6a1a 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.h
> +++ b/tools/testing/selftests/bpf/xskxceiver.h
> @@ -55,7 +55,6 @@
>  #define XSK_UMEM__LARGE_FRAME_SIZE (3 * 1024)
>  #define XSK_UMEM__MAX_FRAME_SIZE (4 * 1024)
>  #define XSK_DESC__INVALID_OPTION (0xffff)
> -#define XSK_DESC__MAX_SKB_FRAGS 18
>  #define HUGEPAGE_SIZE (2 * 1024 * 1024)
>  #define PKT_DUMP_NB_TO_PRINT 16
>  #define RUN_ALL_TESTS UINT_MAX
> --
> 2.34.1
>
>

Maciej Fijalkowski Sept. 10, 2024, noon UTC | #2

On Tue, Sep 10, 2024 at 01:48:35PM +0200, Magnus Karlsson wrote:
> On Mon, 9 Sept 2024 at 16:12, Maciej Fijalkowski
> <[e-mail]> wrote:
> >
> > Currently, xskxceiver assumes that MAX_SKB_FRAGS value is always 17
> > which is not true - since the introduction of BIG TCP this can now take
> > any value between 17 to 45 via CONFIG_MAX_SKB_FRAGS.
> >
> > Adjust the TOO_MANY_FRAGS test case to read the currently configured
> > MAX_SKB_FRAGS value by reading it from /proc/sys/net/core/max_skb_frags.
> >
> > Signed-off-by: Maciej Fijalkowski <[e-mail]>
> > ---
> >  tools/testing/selftests/bpf/xskxceiver.c | 41 +++++++++++++++++++++---
> >  tools/testing/selftests/bpf/xskxceiver.h |  1 -
> >  2 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > index 92af633faea8..595b6da26897 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -325,6 +325,25 @@ static bool ifobj_zc_avail(struct ifobject *ifobject)
> >         return zc_avail;
> >  }
> >
> > +#define MAX_SKB_FRAGS_PATH "/proc/sys/net/core/max_skb_frags"
> > +static unsigned int get_max_skb_frags(void)
> > +{
> > +       unsigned int max_skb_frags = 0;
> > +       FILE *file;
> > +
> > +       file = fopen(MAX_SKB_FRAGS_PATH, "r");
> > +       if (!file) {
> > +               ksft_print_msg("Error opening %s\n", MAX_SKB_FRAGS_PATH);
> > +               return 0;
> > +       }
> > +
> > +       if (fscanf(file, "%u", &max_skb_frags) != 1)
> > +               ksft_print_msg("Error reading %s\n", MAX_SKB_FRAGS_PATH);
> > +
> > +       fclose(file);
> > +       return max_skb_frags;
> > +}
> > +
> >  static struct option long_options[] = {
> >         {"interface", required_argument, 0, 'i'},
> >         {"busy-poll", no_argument, 0, 'b'},
> > @@ -2245,13 +2264,22 @@ static int testapp_poll_rxq_tmout(struct test_spec *test)
> >
> >  static int testapp_too_many_frags(struct test_spec *test)
> >  {
> > -       struct pkt pkts[2 * XSK_DESC__MAX_SKB_FRAGS + 2] = {};
> > +       struct pkt *pkts;
> >         u32 max_frags, i;
> > +       int ret;
> >
> > -       if (test->mode == TEST_MODE_ZC)
> > +       if (test->mode == TEST_MODE_ZC) {
> >                 max_frags = test->ifobj_tx->xdp_zc_max_segs;
> > -       else
> > -               max_frags = XSK_DESC__MAX_SKB_FRAGS;
> > +       } else {
> > +               max_frags = get_max_skb_frags();
> > +               if (!max_frags)
> > +                       return TEST_FAILURE;
> 
> Thanks for this fix Maciej. However, I think failing the test here is
> a little bit too drastic. How about just returning TEST_SKIP and print
> out that the max number of skbs is unknown as the reason for the skip?
> Or even more optimistically, print out a warning that we could not
> read the max number of skb but we are guessing 17 and then run the
> test? If it passes, great we guessed correctly, but if it fails we are
> not worse off than the current code.

makes sense to default to 17 if we couldn't read it from file. will fix in
v2.

> Do not know how often a file
> system does not contain /proc/sys/net/core/max_skb_frags though.

A mix of CONFIG_NET, CONFIG_PROC_FS and CONFIG_SYSCTL i think.

> 
> > +               max_frags += 1;
> > +       }
> > +
> > +       pkts = calloc(2 * max_frags + 2, sizeof(struct pkt));
> > +       if (!pkts)
> > +               return TEST_FAILURE;
> >
> >         test->mtu = MAX_ETH_JUMBO_SIZE;
> >
> > @@ -2281,7 +2309,10 @@ static int testapp_too_many_frags(struct test_spec *test)
> >         pkts[2 * max_frags + 1].valid = true;
> >
> >         pkt_stream_generate_custom(test, pkts, 2 * max_frags + 2);
> > -       return testapp_validate_traffic(test);
> > +       ret = testapp_validate_traffic(test);
> > +
> > +       free(pkts);
> > +       return ret;
> >  }
> >
> >  static int xsk_load_xdp_programs(struct ifobject *ifobj)
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
> > index 885c948c5d83..e46e823f6a1a 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.h
> > +++ b/tools/testing/selftests/bpf/xskxceiver.h
> > @@ -55,7 +55,6 @@
> >  #define XSK_UMEM__LARGE_FRAME_SIZE (3 * 1024)
> >  #define XSK_UMEM__MAX_FRAME_SIZE (4 * 1024)
> >  #define XSK_DESC__INVALID_OPTION (0xffff)
> > -#define XSK_DESC__MAX_SKB_FRAGS 18
> >  #define HUGEPAGE_SIZE (2 * 1024 * 1024)
> >  #define PKT_DUMP_NB_TO_PRINT 16
> >  #define RUN_ALL_TESTS UINT_MAX
> > --
> > 2.34.1
> >
> >

Patch

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 92af633faea8..595b6da26897 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -325,6 +325,25 @@  static bool ifobj_zc_avail(struct ifobject *ifobject)
 	return zc_avail;
 }
 
+#define MAX_SKB_FRAGS_PATH "/proc/sys/net/core/max_skb_frags"
+static unsigned int get_max_skb_frags(void)
+{
+	unsigned int max_skb_frags = 0;
+	FILE *file;
+
+	file = fopen(MAX_SKB_FRAGS_PATH, "r");
+	if (!file) {
+		ksft_print_msg("Error opening %s\n", MAX_SKB_FRAGS_PATH);
+		return 0;
+	}
+
+	if (fscanf(file, "%u", &max_skb_frags) != 1)
+		ksft_print_msg("Error reading %s\n", MAX_SKB_FRAGS_PATH);
+
+	fclose(file);
+	return max_skb_frags;
+}
+
 static struct option long_options[] = {
 	{"interface", required_argument, 0, 'i'},
 	{"busy-poll", no_argument, 0, 'b'},
@@ -2245,13 +2264,22 @@  static int testapp_poll_rxq_tmout(struct test_spec *test)
 
 static int testapp_too_many_frags(struct test_spec *test)
 {
-	struct pkt pkts[2 * XSK_DESC__MAX_SKB_FRAGS + 2] = {};
+	struct pkt *pkts;
 	u32 max_frags, i;
+	int ret;
 
-	if (test->mode == TEST_MODE_ZC)
+	if (test->mode == TEST_MODE_ZC) {
 		max_frags = test->ifobj_tx->xdp_zc_max_segs;
-	else
-		max_frags = XSK_DESC__MAX_SKB_FRAGS;
+	} else {
+		max_frags = get_max_skb_frags();
+		if (!max_frags)
+			return TEST_FAILURE;
+		max_frags += 1;
+	}
+
+	pkts = calloc(2 * max_frags + 2, sizeof(struct pkt));
+	if (!pkts)
+		return TEST_FAILURE;
 
 	test->mtu = MAX_ETH_JUMBO_SIZE;
 
@@ -2281,7 +2309,10 @@  static int testapp_too_many_frags(struct test_spec *test)
 	pkts[2 * max_frags + 1].valid = true;
 
 	pkt_stream_generate_custom(test, pkts, 2 * max_frags + 2);
-	return testapp_validate_traffic(test);
+	ret = testapp_validate_traffic(test);
+
+	free(pkts);
+	return ret;
 }
 
 static int xsk_load_xdp_programs(struct ifobject *ifobj)
diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
index 885c948c5d83..e46e823f6a1a 100644
--- a/tools/testing/selftests/bpf/xskxceiver.h
+++ b/tools/testing/selftests/bpf/xskxceiver.h
@@ -55,7 +55,6 @@ 
 #define XSK_UMEM__LARGE_FRAME_SIZE (3 * 1024)
 #define XSK_UMEM__MAX_FRAME_SIZE (4 * 1024)
 #define XSK_DESC__INVALID_OPTION (0xffff)
-#define XSK_DESC__MAX_SKB_FRAGS 18
 #define HUGEPAGE_SIZE (2 * 1024 * 1024)
 #define PKT_DUMP_NB_TO_PRINT 16
 #define RUN_ALL_TESTS UINT_MAX