Re: [PATCH v6 bpf-next 1/5] bpf: Add bloom filter map implementation

[Joanne Koong <joannekoong@fb.com>]

On 10/28/21 2:14 PM, Martin KaFai Lau wrote:

> On Wed, Oct 27, 2021 at 04:45:00PM -0700, Joanne Koong wrote:
[...]
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index c10820037883..8bead4aa3ad0 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -906,6 +906,7 @@ enum bpf_map_type {
>>   	BPF_MAP_TYPE_RINGBUF,
>>   	BPF_MAP_TYPE_INODE_STORAGE,
>>   	BPF_MAP_TYPE_TASK_STORAGE,
>> +	BPF_MAP_TYPE_BLOOM_FILTER,
>>   };
>>   
>>   /* Note that tracing related programs such as
>> @@ -1274,6 +1275,13 @@ union bpf_attr {
>>   						   * struct stored as the
>>   						   * map value
>>   						   */
>> +		/* Any per-map-type extra fields
>> +		 *
>> +		 * BPF_MAP_TYPE_BLOOM_FILTER - the lowest 4 bits indicate the
>> +		 * number of hash functions (if 0, the bloom filter will default
>> +		 * to using 5 hash functions).
>> +		 */
>> +		__u64	map_extra;
>>   	};
>>   
When I run pahole (on an x86-64 machine), I see that there's an 8 byte hole
right before map_extra in the "union bpf_attr" struct (above this 
paragraph).
It seems like this should be padded as well with a "__u64 :64;"? I will 
add that in.
>>   	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
>> @@ -5638,6 +5646,7 @@ struct bpf_map_info {
>>   	__u32 btf_id;
>>   	__u32 btf_key_type_id;
>>   	__u32 btf_value_type_id;
> There is also a 4 byte hole here.  A "__u32 :32" is needed.
> You can find details in 36f9814a494a ("bpf: fix uapi hole for 32 bit compat applications")
>
>> +	__u64 map_extra;
>>   } __attribute__((aligned(8)));
> [ ... ]
>
>> +static int peek_elem(struct bpf_map *map, void *value)
> These generic map-ops names could be confusing in tracing and
> in perf-report.  There was a 'bloom_filter_map_' prefix in the earlier version.
> I could have missed something in the earlier discussion threads.
> What was the reason in dropping the prefix?
>
The reason I dropped the prefix was so that the function names would be
less verbose. Your point about it being confusing in tracing and in 
perf-report
makes a lot of sense - I will add it back in!

[...]