diff mbox series

[bpf-next,3/3] selftests/bpf: Add bloom map success test for userspace calls

Message ID 20211029170126.4189338-4-joannekoong@fb.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series "map_extra" and bloom filter fixups | expand

Checks

Context Check Description
bpf/vmtest-bpf-next fail VM_Test
bpf/vmtest-bpf-next-PR fail PR summary
netdev/cover_letter success Series has a cover letter
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 7 maintainers not CCed: john.fastabend@gmail.com linux-kselftest@vger.kernel.org yhs@fb.com netdev@vger.kernel.org songliubraving@fb.com kpsingh@kernel.org shuah@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success No Fixes tag
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Joanne Koong Oct. 29, 2021, 5:01 p.m. UTC 
This patch has two changes:
1) Adds a new function "test_success_cases" to test
successfully creating + adding + looking up a value
in a bloom filter map from the userspace side.

2) Use bpf_create_map instead of bpf_create_map_xattr in
the "test_fail_cases" to make the code look cleaner.

Signed-off-by: Joanne Koong <joannekoong@fb.com>
---
 .../bpf/prog_tests/bloom_filter_map.c         | 53 ++++++++++++-------
 1 file changed, 33 insertions(+), 20 deletions(-)

Comments

Yonghong Song Oct. 29, 2021, 10:04 p.m. UTC | #1 
On 10/29/21 10:01 AM, Joanne Koong wrote:
> This patch has two changes:
> 1) Adds a new function "test_success_cases" to test
> successfully creating + adding + looking up a value
> in a bloom filter map from the userspace side.
> 
> 2) Use bpf_create_map instead of bpf_create_map_xattr in
> the "test_fail_cases" to make the code look cleaner.
> 
> Signed-off-by: Joanne Koong <joannekoong@fb.com>

LGTM with one minor comment below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../bpf/prog_tests/bloom_filter_map.c         | 53 ++++++++++++-------
>   1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> index 9aa3fbed918b..dbc0035e43e5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> @@ -7,44 +7,32 @@
>   
>   static void test_fail_cases(void)
>   {
> -	struct bpf_create_map_attr xattr = {
> -		.name = "bloom_filter_map",
> -		.map_type = BPF_MAP_TYPE_BLOOM_FILTER,
> -		.max_entries = 100,
> -		.value_size = 11,
> -	};
>   	__u32 value;
>   	int fd, err;
>   
>   	/* Invalid key size */
> -	xattr.key_size = 4;
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 4, sizeof(value), 100, 0);
>   	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid key size"))
>   		close(fd);
> -	xattr.key_size = 0;
>   
>   	/* Invalid value size */
> -	xattr.value_size = 0;
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, 0, 100, 0);
>   	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid value size 0"))
>   		close(fd);
> -	xattr.value_size = 11;
>   
>   	/* Invalid max entries size */
> -	xattr.max_entries = 0;
> -	fd = bpf_create_map_xattr(&xattr);
> -	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid max entries size"))
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 0, 0);
> +	if (!ASSERT_LT(fd, 0,
> +		       "bpf_create_map bloom filter invalid max entries size"))

It is OK to have "bpf_create_map ..." in the same line as ASSERT_LT
for better readability and consistent with other ASSERT_LT. It is over 
80 but less than 100 char's per line.

>   		close(fd);
> -	xattr.max_entries = 100;
>   
>   	/* Bloom filter maps do not support BPF_F_NO_PREALLOC */
> -	xattr.map_flags = BPF_F_NO_PREALLOC;
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100,
> +			    BPF_F_NO_PREALLOC);
>   	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid flags"))
>   		close(fd);
> -	xattr.map_flags = 0;
>   
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100, 0);
>   	if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter"))
>   		return;
>   
> @@ -67,6 +55,30 @@ static void test_fail_cases(void)
>   	close(fd);
>   }
[...]
Joanne Koong Oct. 29, 2021, 10:30 p.m. UTC | #2 
On 10/29/21 3:04 PM, Yonghong Song wrote:

> On 10/29/21 10:01 AM, Joanne Koong wrote:
>> This patch has two changes:
>> 1) Adds a new function "test_success_cases" to test
>> successfully creating + adding + looking up a value
>> in a bloom filter map from the userspace side.
>>
>> 2) Use bpf_create_map instead of bpf_create_map_xattr in
>> the "test_fail_cases" to make the code look cleaner.
>>
>> Signed-off-by: Joanne Koong <joannekoong@fb.com>
>
> LGTM with one minor comment below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
>> ---
>>   .../bpf/prog_tests/bloom_filter_map.c         | 53 ++++++++++++-------
>>   1 file changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git 
>> a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c 
>> b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
>> index 9aa3fbed918b..dbc0035e43e5 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
>> @@ -7,44 +7,32 @@
>>     static void test_fail_cases(void)
>>   {
>> -    struct bpf_create_map_attr xattr = {
>> -        .name = "bloom_filter_map",
>> -        .map_type = BPF_MAP_TYPE_BLOOM_FILTER,
>> -        .max_entries = 100,
>> -        .value_size = 11,
>> -    };
>>       __u32 value;
>>       int fd, err;
>>         /* Invalid key size */
>> -    xattr.key_size = 4;
>> -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 4, sizeof(value), 
>> 100, 0);
>>       if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid key 
>> size"))
>>           close(fd);
>> -    xattr.key_size = 0;
>>         /* Invalid value size */
>> -    xattr.value_size = 0;
>> -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, 0, 100, 0);
>>       if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid 
>> value size 0"))
>>           close(fd);
>> -    xattr.value_size = 11;
>>         /* Invalid max entries size */
>> -    xattr.max_entries = 0;
>> -    fd = bpf_create_map_xattr(&xattr);
>> -    if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid max 
>> entries size"))
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 
>> 0, 0);
>> +    if (!ASSERT_LT(fd, 0,
>> +               "bpf_create_map bloom filter invalid max entries size"))
>
> It is OK to have "bpf_create_map ..." in the same line as ASSERT_LT
> for better readability and consistent with other ASSERT_LT. It is over 
> 80 but less than 100 char's per line.
>
Great, I will send out v2 of this patchset where this line break is 
removed.
Thanks for reviewing the patchset!
>>           close(fd);
>> -    xattr.max_entries = 100;
>>         /* Bloom filter maps do not support BPF_F_NO_PREALLOC */
>> -    xattr.map_flags = BPF_F_NO_PREALLOC;
>> -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 
>> 100,
>> +                BPF_F_NO_PREALLOC);
>>       if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid 
>> flags"))
>>           close(fd);
>> -    xattr.map_flags = 0;
>>   -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 
>> 100, 0);
>>       if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter"))
>>           return;
>>   @@ -67,6 +55,30 @@ static void test_fail_cases(void)
>>       close(fd);
>>   }
> [...]
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
index 9aa3fbed918b..dbc0035e43e5 100644
--- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
@@ -7,44 +7,32 @@ 
 
 static void test_fail_cases(void)
 {
-	struct bpf_create_map_attr xattr = {
-		.name = "bloom_filter_map",
-		.map_type = BPF_MAP_TYPE_BLOOM_FILTER,
-		.max_entries = 100,
-		.value_size = 11,
-	};
 	__u32 value;
 	int fd, err;
 
 	/* Invalid key size */
-	xattr.key_size = 4;
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 4, sizeof(value), 100, 0);
 	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid key size"))
 		close(fd);
-	xattr.key_size = 0;
 
 	/* Invalid value size */
-	xattr.value_size = 0;
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, 0, 100, 0);
 	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid value size 0"))
 		close(fd);
-	xattr.value_size = 11;
 
 	/* Invalid max entries size */
-	xattr.max_entries = 0;
-	fd = bpf_create_map_xattr(&xattr);
-	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid max entries size"))
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 0, 0);
+	if (!ASSERT_LT(fd, 0,
+		       "bpf_create_map bloom filter invalid max entries size"))
 		close(fd);
-	xattr.max_entries = 100;
 
 	/* Bloom filter maps do not support BPF_F_NO_PREALLOC */
-	xattr.map_flags = BPF_F_NO_PREALLOC;
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100,
+			    BPF_F_NO_PREALLOC);
 	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid flags"))
 		close(fd);
-	xattr.map_flags = 0;
 
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100, 0);
 	if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter"))
 		return;
 
@@ -67,6 +55,30 @@  static void test_fail_cases(void)
 	close(fd);
 }
 
+static void test_success_cases(void)
+{
+	char value[11];
+	int fd, err;
+
+	/* Create a map */
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100,
+			    BPF_F_ZERO_SEED | BPF_F_NUMA_NODE);
+	if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter success case"))
+		return;
+
+	/* Add a value to the bloom filter */
+	err = bpf_map_update_elem(fd, NULL, &value, 0);
+	if (!ASSERT_OK(err, "bpf_map_update_elem bloom filter success case"))
+		goto done;
+
+	 /* Lookup a value in the bloom filter */
+	err = bpf_map_lookup_elem(fd, NULL, &value);
+	ASSERT_OK(err, "bpf_map_update_elem bloom filter success case");
+
+done:
+	close(fd);
+}
+
 static void check_bloom(struct bloom_filter_map *skel)
 {
 	struct bpf_link *link;
@@ -190,6 +202,7 @@  void test_bloom_filter_map(void)
 	int err;
 
 	test_fail_cases();
+	test_success_cases();
 
 	err = setup_progs(&skel, &rand_vals, &nr_rand_vals);
 	if (err)