* [PATCH net-next v2 1/3] net: add skb_segment kunit test
2023-10-08 20:12 [PATCH net-next v2 0/3] add skb_segment kunit coverage Willem de Bruijn
@ 2023-10-08 20:12 ` Willem de Bruijn
2023-10-09 4:01 ` kernel test robot
2023-10-08 20:12 ` [PATCH net-next v2 2/3] net: parametrize skb_segment unit test to expand coverage Willem de Bruijn
2023-10-08 20:12 ` [PATCH net-next v2 3/3] net: expand skb_segment unit test with frag_list coverage Willem de Bruijn
2 siblings, 1 reply; 5+ messages in thread
From: Willem de Bruijn @ 2023-10-08 20:12 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, edumazet, pabeni, alexander.duyck, fw,
Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Add unit testing for skb segment. This function is exercised by many
different code paths, such as GSO_PARTIAL or GSO_BY_FRAGS, linear
(with or without head_frag), frags or frag_list skbs, etc.
It is infeasible to manually run tests that cover all code paths when
making changes. The long and complex function also makes it hard to
establish through analysis alone that a patch has no unintended
side-effects.
Add code coverage through kunit regression testing. Introduce kunit
infrastructure for tests under net/core, and add this first test.
This first skb_segment test exercises a simple case: a linear skb.
Follow-on patches will parametrize the test and add more variants.
Tested: Built and ran the test with
make ARCH=um mrproper
./tools/testing/kunit/kunit.py run \
--kconfig_add CONFIG_NET=y \
--kconfig_add CONFIG_DEBUG_KERNEL=y \
--kconfig_add CONFIG_DEBUG_INFO=y \
--kconfig_add=CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y \
net_core_gso
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
v1->v2
- add MODULE_DESCRIPTION
---
net/Kconfig | 9 +++++
net/core/Makefile | 1 +
net/core/gso_test.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 97 insertions(+)
create mode 100644 net/core/gso_test.c
diff --git a/net/Kconfig b/net/Kconfig
index d532ec33f1fed..17676dba10fbe 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -508,4 +508,13 @@ config NETDEV_ADDR_LIST_TEST
default KUNIT_ALL_TESTS
depends on KUNIT
+config NET_TEST
+ tristate "KUnit tests for networking" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ KUnit tests covering core networking infra, such as sk_buff.
+
+ If unsure, say N.
+
endif # if NET
diff --git a/net/core/Makefile b/net/core/Makefile
index 731db2eaa6107..0cb734cbc24b2 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -40,3 +40,4 @@ obj-$(CONFIG_NET_SOCK_MSG) += skmsg.o
obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
obj-$(CONFIG_OF) += of_net.o
+obj-$(CONFIG_NET_TEST) += gso_test.o
diff --git a/net/core/gso_test.c b/net/core/gso_test.c
new file mode 100644
index 0000000000000..eaa85939ed208
--- /dev/null
+++ b/net/core/gso_test.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <kunit/test.h>
+#include <linux/skbuff.h>
+
+static const char hdr[] = "abcdefgh";
+static const int gso_size = 1000, last_seg_size = 1;
+
+/* default: create 3 segment gso packet */
+static const int payload_len = (2 * gso_size) + last_seg_size;
+
+static void __init_skb(struct sk_buff *skb)
+{
+ skb_reset_mac_header(skb);
+ memcpy(skb_mac_header(skb), hdr, sizeof(hdr));
+
+ /* skb_segment expects skb->data at start of payload */
+ skb_pull(skb, sizeof(hdr));
+ skb_reset_network_header(skb);
+ skb_reset_transport_header(skb);
+
+ /* proto is arbitrary, as long as not ETH_P_TEB or vlan */
+ skb->protocol = htons(ETH_P_ATALK);
+ skb_shinfo(skb)->gso_size = gso_size;
+}
+
+static void gso_test_func(struct kunit *test)
+{
+ const int shinfo_size = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+ struct sk_buff *skb, *segs, *cur;
+ struct page *page;
+
+ page = alloc_page(GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, page);
+ skb = build_skb(page_address(page), sizeof(hdr) + payload_len + shinfo_size);
+ KUNIT_ASSERT_NOT_NULL(test, skb);
+ __skb_put(skb, sizeof(hdr) + payload_len);
+
+ __init_skb(skb);
+
+ segs = skb_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM);
+ if (IS_ERR(segs)) {
+ KUNIT_FAIL(test, "segs error %lld", PTR_ERR(segs));
+ goto free_gso_skb;
+ } else if (!segs) {
+ KUNIT_FAIL(test, "no segments");
+ goto free_gso_skb;
+ }
+
+ for (cur = segs; cur; cur = cur->next) {
+ /* segs have skb->data pointing to the mac header */
+ KUNIT_ASSERT_PTR_EQ(test, skb_mac_header(cur), cur->data);
+ KUNIT_ASSERT_PTR_EQ(test, skb_network_header(cur), cur->data + sizeof(hdr));
+
+ /* header was copied to all segs */
+ KUNIT_ASSERT_EQ(test, memcmp(skb_mac_header(cur), hdr, sizeof(hdr)), 0);
+
+ /* all segs are gso_size, except for last */
+ if (cur->next) {
+ KUNIT_ASSERT_EQ(test, cur->len, sizeof(hdr) + gso_size);
+ } else {
+ KUNIT_ASSERT_EQ(test, cur->len, sizeof(hdr) + last_seg_size);
+
+ /* last seg can be found through segs->prev pointer */
+ KUNIT_ASSERT_PTR_EQ(test, cur, segs->prev);
+ }
+ }
+
+ consume_skb(segs);
+free_gso_skb:
+ consume_skb(skb);
+}
+
+static struct kunit_case gso_test_cases[] = {
+ KUNIT_CASE(gso_test_func),
+ {}
+};
+
+static struct kunit_suite gso_test_suite = {
+ .name = "net_core_gso",
+ .test_cases = gso_test_cases,
+};
+
+kunit_test_suite(gso_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("KUnit tests for segmentation offload");
--
2.42.0.609.gbb76f46606-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next v2 1/3] net: add skb_segment kunit test
2023-10-08 20:12 ` [PATCH net-next v2 1/3] net: add skb_segment kunit test Willem de Bruijn
@ 2023-10-09 4:01 ` kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2023-10-09 4:01 UTC (permalink / raw)
To: Willem de Bruijn, netdev
Cc: oe-kbuild-all, davem, kuba, edumazet, pabeni, alexander.duyck, fw,
Willem de Bruijn
Hi Willem,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Willem-de-Bruijn/net-add-skb_segment-kunit-test/20231009-041424
base: net-next/main
patch link: https://lore.kernel.org/r/20231008201244.3700784-2-willemdebruijn.kernel%40gmail.com
patch subject: [PATCH net-next v2 1/3] net: add skb_segment kunit test
config: i386-randconfig-001-20231009 (https://download.01.org/0day-ci/archive/20231009/202310091154.3StA08FY-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231009/202310091154.3StA08FY-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310091154.3StA08FY-lkp@intel.com/
All errors (new ones prefixed by >>):
>> net/core/gso_test.c:10:32: error: initializer element is not constant
static const int payload_len = (2 * gso_size) + last_seg_size;
^
vim +10 net/core/gso_test.c
8
9 /* default: create 3 segment gso packet */
> 10 static const int payload_len = (2 * gso_size) + last_seg_size;
11
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net-next v2 2/3] net: parametrize skb_segment unit test to expand coverage
2023-10-08 20:12 [PATCH net-next v2 0/3] add skb_segment kunit coverage Willem de Bruijn
2023-10-08 20:12 ` [PATCH net-next v2 1/3] net: add skb_segment kunit test Willem de Bruijn
@ 2023-10-08 20:12 ` Willem de Bruijn
2023-10-08 20:12 ` [PATCH net-next v2 3/3] net: expand skb_segment unit test with frag_list coverage Willem de Bruijn
2 siblings, 0 replies; 5+ messages in thread
From: Willem de Bruijn @ 2023-10-08 20:12 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, edumazet, pabeni, alexander.duyck, fw,
Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Expand the test with variants
- GSO_TEST_NO_GSO: payload size less than or equal to gso_size
- GSO_TEST_FRAGS: payload in both linear and page frags
- GSO_TEST_FRAGS_PURE: payload exclusively in page frags
- GSO_TEST_GSO_PARTIAL: produce one gso segment of multiple of gso_size,
plus optionally one non-gso trailer segment
Define a test struct that encodes the input gso skb and output segs.
Input in terms of linear and fragment lengths. Output as length of
each segment.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
net/core/gso_test.c | 129 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 112 insertions(+), 17 deletions(-)
diff --git a/net/core/gso_test.c b/net/core/gso_test.c
index eaa85939ed208..c4e0b0832dbac 100644
--- a/net/core/gso_test.c
+++ b/net/core/gso_test.c
@@ -4,10 +4,7 @@
#include <linux/skbuff.h>
static const char hdr[] = "abcdefgh";
-static const int gso_size = 1000, last_seg_size = 1;
-
-/* default: create 3 segment gso packet */
-static const int payload_len = (2 * gso_size) + last_seg_size;
+static const int gso_size = 1000;
static void __init_skb(struct sk_buff *skb)
{
@@ -24,21 +21,121 @@ static void __init_skb(struct sk_buff *skb)
skb_shinfo(skb)->gso_size = gso_size;
}
+enum gso_test_nr {
+ GSO_TEST_LINEAR,
+ GSO_TEST_NO_GSO,
+ GSO_TEST_FRAGS,
+ GSO_TEST_FRAGS_PURE,
+ GSO_TEST_GSO_PARTIAL,
+};
+
+struct gso_test_case {
+ enum gso_test_nr id;
+ const char *name;
+
+ /* input */
+ unsigned int linear_len;
+ unsigned int nr_frags;
+ const unsigned int *frags;
+
+ /* output as expected */
+ unsigned int nr_segs;
+ const unsigned int *segs;
+};
+
+static struct gso_test_case cases[] = {
+ {
+ .id = GSO_TEST_NO_GSO,
+ .name = "no_gso",
+ .linear_len = gso_size,
+ .nr_segs = 1,
+ .segs = (const unsigned int[]) { gso_size },
+ },
+ {
+ .id = GSO_TEST_LINEAR,
+ .name = "linear",
+ .linear_len = gso_size + gso_size + 1,
+ .nr_segs = 3,
+ .segs = (const unsigned int[]) { gso_size, gso_size, 1 },
+ },
+ {
+ .id = GSO_TEST_FRAGS,
+ .name = "frags",
+ .linear_len = gso_size,
+ .nr_frags = 2,
+ .frags = (const unsigned int[]) { gso_size, 1 },
+ .nr_segs = 3,
+ .segs = (const unsigned int[]) { gso_size, gso_size, 1 },
+ },
+ {
+ .id = GSO_TEST_FRAGS_PURE,
+ .name = "frags_pure",
+ .nr_frags = 3,
+ .frags = (const unsigned int[]) { gso_size, gso_size, 2 },
+ .nr_segs = 3,
+ .segs = (const unsigned int[]) { gso_size, gso_size, 2 },
+ },
+ {
+ .id = GSO_TEST_GSO_PARTIAL,
+ .name = "gso_partial",
+ .linear_len = gso_size,
+ .nr_frags = 2,
+ .frags = (const unsigned int[]) { gso_size, 3 },
+ .nr_segs = 2,
+ .segs = (const unsigned int[]) { 2 * gso_size, 3 },
+ },
+};
+
+static void gso_test_case_to_desc(struct gso_test_case *t, char *desc)
+{
+ sprintf(desc, "%s", t->name);
+}
+
+KUNIT_ARRAY_PARAM(gso_test, cases, gso_test_case_to_desc);
+
static void gso_test_func(struct kunit *test)
{
const int shinfo_size = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+ const struct gso_test_case *tcase;
struct sk_buff *skb, *segs, *cur;
+ netdev_features_t features;
struct page *page;
+ int i;
+
+ tcase = test->param_value;
page = alloc_page(GFP_KERNEL);
KUNIT_ASSERT_NOT_NULL(test, page);
- skb = build_skb(page_address(page), sizeof(hdr) + payload_len + shinfo_size);
+ skb = build_skb(page_address(page), sizeof(hdr) + tcase->linear_len + shinfo_size);
KUNIT_ASSERT_NOT_NULL(test, skb);
- __skb_put(skb, sizeof(hdr) + payload_len);
+ __skb_put(skb, sizeof(hdr) + tcase->linear_len);
__init_skb(skb);
- segs = skb_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM);
+ if (tcase->nr_frags) {
+ unsigned int pg_off = 0;
+
+ page = alloc_page(GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, page);
+ page_ref_add(page, tcase->nr_frags - 1);
+
+ for (i = 0; i < tcase->nr_frags; i++) {
+ skb_fill_page_desc(skb, i, page, pg_off, tcase->frags[i]);
+ pg_off += tcase->frags[i];
+ }
+
+ KUNIT_ASSERT_LE(test, pg_off, PAGE_SIZE);
+
+ skb->data_len = pg_off;
+ skb->len += skb->data_len;
+ skb->truesize += skb->data_len;
+ }
+
+ features = NETIF_F_SG | NETIF_F_HW_CSUM;
+ if (tcase->id == GSO_TEST_GSO_PARTIAL)
+ features |= NETIF_F_GSO_PARTIAL;
+
+ segs = skb_segment(skb, features);
if (IS_ERR(segs)) {
KUNIT_FAIL(test, "segs error %lld", PTR_ERR(segs));
goto free_gso_skb;
@@ -47,7 +144,9 @@ static void gso_test_func(struct kunit *test)
goto free_gso_skb;
}
- for (cur = segs; cur; cur = cur->next) {
+ for (cur = segs, i = 0; cur; cur = cur->next, i++) {
+ KUNIT_ASSERT_EQ(test, cur->len, sizeof(hdr) + tcase->segs[i]);
+
/* segs have skb->data pointing to the mac header */
KUNIT_ASSERT_PTR_EQ(test, skb_mac_header(cur), cur->data);
KUNIT_ASSERT_PTR_EQ(test, skb_network_header(cur), cur->data + sizeof(hdr));
@@ -55,24 +154,20 @@ static void gso_test_func(struct kunit *test)
/* header was copied to all segs */
KUNIT_ASSERT_EQ(test, memcmp(skb_mac_header(cur), hdr, sizeof(hdr)), 0);
- /* all segs are gso_size, except for last */
- if (cur->next) {
- KUNIT_ASSERT_EQ(test, cur->len, sizeof(hdr) + gso_size);
- } else {
- KUNIT_ASSERT_EQ(test, cur->len, sizeof(hdr) + last_seg_size);
-
- /* last seg can be found through segs->prev pointer */
+ /* last seg can be found through segs->prev pointer */
+ if (!cur->next)
KUNIT_ASSERT_PTR_EQ(test, cur, segs->prev);
- }
}
+ KUNIT_ASSERT_EQ(test, i, tcase->nr_segs);
+
consume_skb(segs);
free_gso_skb:
consume_skb(skb);
}
static struct kunit_case gso_test_cases[] = {
- KUNIT_CASE(gso_test_func),
+ KUNIT_CASE_PARAM(gso_test_func, gso_test_gen_params),
{}
};
--
2.42.0.609.gbb76f46606-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net-next v2 3/3] net: expand skb_segment unit test with frag_list coverage
2023-10-08 20:12 [PATCH net-next v2 0/3] add skb_segment kunit coverage Willem de Bruijn
2023-10-08 20:12 ` [PATCH net-next v2 1/3] net: add skb_segment kunit test Willem de Bruijn
2023-10-08 20:12 ` [PATCH net-next v2 2/3] net: parametrize skb_segment unit test to expand coverage Willem de Bruijn
@ 2023-10-08 20:12 ` Willem de Bruijn
2 siblings, 0 replies; 5+ messages in thread
From: Willem de Bruijn @ 2023-10-08 20:12 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, edumazet, pabeni, alexander.duyck, fw,
Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Expand the test with these variants that use skb frag_list:
- GSO_TEST_FRAG_LIST: frag_skb length is gso_size
- GSO_TEST_FRAG_LIST_PURE: same, data exclusively in frag skbs
- GSO_TEST_FRAG_LIST_NON_UNIFORM: frag_skb length may vary
- GSO_TEST_GSO_BY_FRAGS: frag_skb length defines gso_size,
i.e., segs may have varying sizes.
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
v1->v2
- maintain reverse christmas tree
---
net/core/gso_test.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
diff --git a/net/core/gso_test.c b/net/core/gso_test.c
index c4e0b0832dbac..c1a6cffb6f961 100644
--- a/net/core/gso_test.c
+++ b/net/core/gso_test.c
@@ -27,6 +27,10 @@ enum gso_test_nr {
GSO_TEST_FRAGS,
GSO_TEST_FRAGS_PURE,
GSO_TEST_GSO_PARTIAL,
+ GSO_TEST_FRAG_LIST,
+ GSO_TEST_FRAG_LIST_PURE,
+ GSO_TEST_FRAG_LIST_NON_UNIFORM,
+ GSO_TEST_GSO_BY_FRAGS,
};
struct gso_test_case {
@@ -37,6 +41,8 @@ struct gso_test_case {
unsigned int linear_len;
unsigned int nr_frags;
const unsigned int *frags;
+ unsigned int nr_frag_skbs;
+ const unsigned int *frag_skbs;
/* output as expected */
unsigned int nr_segs;
@@ -84,6 +90,48 @@ static struct gso_test_case cases[] = {
.nr_segs = 2,
.segs = (const unsigned int[]) { 2 * gso_size, 3 },
},
+ {
+ /* commit 89319d3801d1: frag_list on mss boundaries */
+ .id = GSO_TEST_FRAG_LIST,
+ .name = "frag_list",
+ .linear_len = gso_size,
+ .nr_frag_skbs = 2,
+ .frag_skbs = (const unsigned int[]) { gso_size, gso_size },
+ .nr_segs = 3,
+ .segs = (const unsigned int[]) { gso_size, gso_size, gso_size },
+ },
+ {
+ .id = GSO_TEST_FRAG_LIST_PURE,
+ .name = "frag_list_pure",
+ .nr_frag_skbs = 2,
+ .frag_skbs = (const unsigned int[]) { gso_size, gso_size },
+ .nr_segs = 2,
+ .segs = (const unsigned int[]) { gso_size, gso_size },
+ },
+ {
+ /* commit 43170c4e0ba7: GRO of frag_list trains */
+ .id = GSO_TEST_FRAG_LIST_NON_UNIFORM,
+ .name = "frag_list_non_uniform",
+ .linear_len = gso_size,
+ .nr_frag_skbs = 4,
+ .frag_skbs = (const unsigned int[]) { gso_size, 1, gso_size, 2 },
+ .nr_segs = 4,
+ .segs = (const unsigned int[]) { gso_size, gso_size, gso_size, 3 },
+ },
+ {
+ /* commit 3953c46c3ac7 ("sk_buff: allow segmenting based on frag sizes") and
+ * commit 90017accff61 ("sctp: Add GSO support")
+ *
+ * "there will be a cover skb with protocol headers and
+ * children ones containing the actual segments"
+ */
+ .id = GSO_TEST_GSO_BY_FRAGS,
+ .name = "gso_by_frags",
+ .nr_frag_skbs = 4,
+ .frag_skbs = (const unsigned int[]) { 100, 200, 300, 400 },
+ .nr_segs = 4,
+ .segs = (const unsigned int[]) { 100, 200, 300, 400 },
+ },
};
static void gso_test_case_to_desc(struct gso_test_case *t, char *desc)
@@ -131,10 +179,54 @@ static void gso_test_func(struct kunit *test)
skb->truesize += skb->data_len;
}
+ if (tcase->frag_skbs) {
+ unsigned int total_size = 0, total_true_size = 0, alloc_size = 0;
+ struct sk_buff *frag_skb, *prev = NULL;
+
+ page = alloc_page(GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, page);
+ page_ref_add(page, tcase->nr_frag_skbs - 1);
+
+ for (i = 0; i < tcase->nr_frag_skbs; i++) {
+ unsigned int frag_size;
+
+ frag_size = tcase->frag_skbs[i];
+ frag_skb = build_skb(page_address(page) + alloc_size,
+ frag_size + shinfo_size);
+ KUNIT_ASSERT_NOT_NULL(test, frag_skb);
+ __skb_put(frag_skb, frag_size);
+
+ if (prev)
+ prev->next = frag_skb;
+ else
+ skb_shinfo(skb)->frag_list = frag_skb;
+ prev = frag_skb;
+
+ total_size += frag_size;
+ total_true_size += frag_skb->truesize;
+ alloc_size += frag_size + shinfo_size;
+ }
+
+ KUNIT_ASSERT_LE(test, alloc_size, PAGE_SIZE);
+
+ skb->len += total_size;
+ skb->data_len += total_size;
+ skb->truesize += total_true_size;
+
+ if (tcase->id == GSO_TEST_GSO_BY_FRAGS)
+ skb_shinfo(skb)->gso_size = GSO_BY_FRAGS;
+ }
+
features = NETIF_F_SG | NETIF_F_HW_CSUM;
if (tcase->id == GSO_TEST_GSO_PARTIAL)
features |= NETIF_F_GSO_PARTIAL;
+ /* TODO: this should also work with SG,
+ * rather than hit BUG_ON(i >= nfrags)
+ */
+ if (tcase->id == GSO_TEST_FRAG_LIST_NON_UNIFORM)
+ features &= ~NETIF_F_SG;
+
segs = skb_segment(skb, features);
if (IS_ERR(segs)) {
KUNIT_FAIL(test, "segs error %lld", PTR_ERR(segs));
--
2.42.0.609.gbb76f46606-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread