Re: [PATCH bpf-next 2/2] bpf: flexible size for bpf_prog_pack

[Song Liu <songliubraving@fb.com>]

> On Feb 10, 2022, at 12:25 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
> On 2/10/22 7:41 AM, Song Liu wrote:
>> bpf_prog_pack uses huge pages to reduce pressue on instruction TLB.
>> To guarantee allocating huge pages for bpf_prog_pack, it is necessary to
>> allocate memory of size PMD_SIZE * num_online_nodes().
>> On the other hand, if the system doesn't support huge pages, it is more
>> efficient to allocate PAGE_SIZE bpf_prog_pack.
>> Address different scenarios with more flexible bpf_prog_pack_size().
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>>  kernel/bpf/core.c | 47 +++++++++++++++++++++++++++--------------------
>>  1 file changed, 27 insertions(+), 20 deletions(-)
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index 42d96549a804..d961a1f07a13 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
>> @@ -814,46 +814,53 @@ int bpf_jit_add_poke_descriptor(struct bpf_prog *prog,
>>   * allocator. The prog_pack allocator uses HPAGE_PMD_SIZE page (2MB on x86)
>>   * to host BPF programs.
>>   */
>> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> -#define BPF_PROG_PACK_SIZE	HPAGE_PMD_SIZE
>> -#else
>> -#define BPF_PROG_PACK_SIZE	PAGE_SIZE
>> -#endif
>>  #define BPF_PROG_CHUNK_SHIFT	6
>>  #define BPF_PROG_CHUNK_SIZE	(1 << BPF_PROG_CHUNK_SHIFT)
>>  #define BPF_PROG_CHUNK_MASK	(~(BPF_PROG_CHUNK_SIZE - 1))
>> -#define BPF_PROG_CHUNK_COUNT	(BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE)
>>    struct bpf_prog_pack {
>>  	struct list_head list;
>>  	void *ptr;
>> -	unsigned long bitmap[BITS_TO_LONGS(BPF_PROG_CHUNK_COUNT)];
>> +	unsigned long bitmap[];
>>  };
>>  -#define BPF_PROG_MAX_PACK_PROG_SIZE	BPF_PROG_PACK_SIZE
>>  #define BPF_PROG_SIZE_TO_NBITS(size)	(round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE)
>>    static DEFINE_MUTEX(pack_mutex);
>>  static LIST_HEAD(pack_list);
>>  +static inline int bpf_prog_pack_size(void)
>> +{
>> +	/* If vmap_allow_huge == true, use pack size of the smallest
>> +	 * possible vmalloc huge page: PMD_SIZE * num_online_nodes().
>> +	 * Otherwise, use pack size of PAGE_SIZE.
>> +	 */
>> +	return get_vmap_allow_huge() ? PMD_SIZE * num_online_nodes() : PAGE_SIZE;
>> +}
> 
> Imho, this is making too many assumptions about implementation details. Can't we
> just add a new module_alloc*() API instead which internally guarantees allocating
> huge pages when enabled/supported (e.g. with a __weak function as fallback)?

I agree that this is making too many assumptions. But a new module_alloc_huge() 
may not work, because we need the caller to know the proper size to ask for. 
(Or maybe I misunderstood your suggestion?)

How about we introduce something like 

    /* minimal size to get huge pages from vmalloc. If not possible, 
     * return 0 (or -1?)
     */
    int vmalloc_hpage_min_size(void)
    {
        return vmap_allow_huge ? PMD_SIZE * num_online_nodes() : 0;
    } 

    /* minimal size to get huge pages from module_alloc */
    int module_alloc_hpage_min_size(void)
    {
        return vmalloc_hpage_min_size();
    }

    static inline int bpf_prog_pack_size(void)
    {
        return module_alloc_hpage_min_size() ? : PAGE_SIZE;
    }

Thanks,
Song