Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1198869 > unrolled thread

[PATCH 2/6] test_bpf: allow tests to specify an skb fragment.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/6] test_bpf: allow tests to specify an skb fragment. Nicolas Schichan <nschichan@freebox.fr> - 2015-08-03 16:10 +0200
    Re: [PATCH 2/6] test_bpf: allow tests to specify an skb fragment. Daniel Borkmann <daniel@iogearbox.net> - 2015-08-03 17:30 +0200
      Re: [PATCH 2/6] test_bpf: allow tests to specify an skb fragment. Nicolas Schichan <nschichan@freebox.fr> - 2015-08-03 18:40 +0200
        Re: [PATCH 2/6] test_bpf: allow tests to specify an skb fragment. Daniel Borkmann <daniel@iogearbox.net> - 2015-08-03 19:40 +0200

#1198869 — [PATCH 2/6] test_bpf: allow tests to specify an skb fragment.

This introduce a new test->aux flag (FLAG_SKB_FRAG) to tell the
populate_skb() function to add a fragment to the test skb containing
the data specified in test->frag_data).

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
 lib/test_bpf.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index 6843d0b..acef1e9 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -23,6 +23,7 @@
 #include <linux/netdevice.h>
 #include <linux/if_vlan.h>
 #include <linux/random.h>
+#include <linux/highmem.h>
 
 /* General test specific settings */
 #define MAX_SUBTESTS	3
@@ -56,6 +57,7 @@
 /* Flags that can be passed to test cases */
 #define FLAG_NO_DATA		BIT(0)
 #define FLAG_EXPECTED_FAIL	BIT(1)
+#define FLAG_SKB_FRAG		BIT(2)
 
 enum {
 	CLASSIC  = BIT(6),	/* Old BPF instructions only. */
@@ -81,6 +83,7 @@ struct bpf_test {
 		__u32 result;
 	} test[MAX_SUBTESTS];
 	int (*fill_helper)(struct bpf_test *self);
+	__u8 frag_data[MAX_DATA];
 };
 
 /* Large test cases need separate allocation and fill handler. */
@@ -4525,6 +4528,10 @@ static struct sk_buff *populate_skb(char *buf, int size)
 
 static void *generate_test_data(struct bpf_test *test, int sub)
 {
+	struct sk_buff *skb;
+	struct page *page;
+	void *ptr;
+
 	if (test->aux & FLAG_NO_DATA)
 		return NULL;
 
@@ -4532,7 +4539,36 @@ static void *generate_test_data(struct bpf_test *test, int sub)
 	 * subtests generate skbs of different sizes based on
 	 * the same data.
 	 */
-	return populate_skb(test->data, test->test[sub].data_size);
+	skb = populate_skb(test->data, test->test[sub].data_size);
+	if (!skb)
+		return NULL;
+
+	if (test->aux & FLAG_SKB_FRAG) {
+		/*
+		 * when the test requires a fragmented skb, add a
+		 * single fragment to the skb, filled with
+		 * test->frag_data.
+		 */
+		page = alloc_page(GFP_KERNEL);
+
+		if (!page)
+			goto err_kfree_skb;
+
+		ptr = kmap(page);
+		if (!ptr)
+			goto err_free_page;
+		memcpy(ptr, test->frag_data, MAX_DATA);
+		kunmap(page);
+		skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
+	}
+
+	return skb;
+
+err_free_page:
+	__free_page(page);
+err_kfree_skb:
+	kfree_skb(skb);
+	return NULL;
 }
 
 static void release_test_data(const struct bpf_test *test, void *data)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1198944

On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> This introduce a new test->aux flag (FLAG_SKB_FRAG) to tell the
> populate_skb() function to add a fragment to the test skb containing
> the data specified in test->frag_data).
>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

I'm good with this change here, just a comment below in general.

>   enum {
>   	CLASSIC  = BIT(6),	/* Old BPF instructions only. */
> @@ -81,6 +83,7 @@ struct bpf_test {
>   		__u32 result;
>   	} test[MAX_SUBTESTS];
>   	int (*fill_helper)(struct bpf_test *self);
> +	__u8 frag_data[MAX_DATA];
>   };

We now have 286 tests, which is awesome!

Perhaps, we need to start thinking of a better test description method
soonish as the test_bpf.ko module grew to ~1.6M, i.e. whenever we add
to struct bpf_test, it adds memory overhead upon all test cases.

>   /* Large test cases need separate allocation and fill handler. */
> @@ -4525,6 +4528,10 @@ static struct sk_buff *populate_skb(char *buf, int size)
>
>   static void *generate_test_data(struct bpf_test *test, int sub)
>   {
> +	struct sk_buff *skb;
> +	struct page *page;
> +	void *ptr;
> +
>   	if (test->aux & FLAG_NO_DATA)
>   		return NULL;
>
> @@ -4532,7 +4539,36 @@ static void *generate_test_data(struct bpf_test *test, int sub)
>   	 * subtests generate skbs of different sizes based on
>   	 * the same data.
>   	 */
> -	return populate_skb(test->data, test->test[sub].data_size);
> +	skb = populate_skb(test->data, test->test[sub].data_size);
> +	if (!skb)
> +		return NULL;
> +
> +	if (test->aux & FLAG_SKB_FRAG) {

Really minor nit: declaration of page, ptr could have been only in this block.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199026

On 08/03/2015 05:29 PM, Daniel Borkmann wrote:
> On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> We now have 286 tests, which is awesome!
> 
> Perhaps, we need to start thinking of a better test description method
> soonish as the test_bpf.ko module grew to ~1.6M, i.e. whenever we add
> to struct bpf_test, it adds memory overhead upon all test cases.

Indeed, test_bpf.ko is turning quite large (1.4M when compiled for ARM).

It looks like gzip is able to do wonders on the module though as I end up with
a 94.7K test_bpf.ko.gz file and if the modutils are compiled with
--enable-zlib, it will be gunziped automatically before being loaded to the
kernel.

I think that marking tests[] array as __initdata will help with the runtime
memory use if someone forgets to rmmod the test_bpf module after a completely
successful run.

>>   /* Large test cases need separate allocation and fill handler. */
>> @@ -4525,6 +4528,10 @@ static struct sk_buff *populate_skb(char *buf, int size)
>>
>>   static void *generate_test_data(struct bpf_test *test, int sub)
>>   {
>> +    struct sk_buff *skb;
>> +    struct page *page;
>> +    void *ptr;
>> +
>>       if (test->aux & FLAG_NO_DATA)
>>           return NULL;
>>
>> @@ -4532,7 +4539,36 @@ static void *generate_test_data(struct bpf_test
>> *test, int sub)
>>        * subtests generate skbs of different sizes based on
>>        * the same data.
>>        */
>> -    return populate_skb(test->data, test->test[sub].data_size);
>> +    skb = populate_skb(test->data, test->test[sub].data_size);
>> +    if (!skb)
>> +        return NULL;
>> +
>> +    if (test->aux & FLAG_SKB_FRAG) {
> 
> Really minor nit: declaration of page, ptr could have been only in this block.

I can certainly move the ptr declaration in the if block, but I'd rather leave
the struct page there to avoid to have the cleanup code awkwardly sitting in
the if block if that's okay with you.

Thanks,

-- 
Nicolas Schichan
Freebox SAS
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199117

On 08/03/2015 06:38 PM, Nicolas Schichan wrote:
> On 08/03/2015 05:29 PM, Daniel Borkmann wrote:
>> On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
>> We now have 286 tests, which is awesome!
>>
>> Perhaps, we need to start thinking of a better test description method
>> soonish as the test_bpf.ko module grew to ~1.6M, i.e. whenever we add
>> to struct bpf_test, it adds memory overhead upon all test cases.
>
> Indeed, test_bpf.ko is turning quite large (1.4M when compiled for ARM).
>
> It looks like gzip is able to do wonders on the module though as I end up with
> a 94.7K test_bpf.ko.gz file and if the modutils are compiled with
> --enable-zlib, it will be gunziped automatically before being loaded to the
> kernel.

I think it just contains a lot of zero blocks, which then compress nicely.

> I think that marking tests[] array as __initdata will help with the runtime
> memory use if someone forgets to rmmod the test_bpf module after a completely
> successful run.

Can be done, too, yep. Do you want to send a patch? ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web