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

[Joanne Koong <joannekoong@fb.com>]

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);
>>   }
> [...]