Re: [PATCH bpf 1/2] vmalloc: replace VM_NO_HUGE_VMAP with VM_ALLOW_HUGE_VMAP

Re: [PATCH bpf 1/2] vmalloc: replace VM_NO_HUGE_VMAP with VM_ALLOW_HUGE_VMAP

[Christoph Hellwig]

On Fri, Apr 08, 2022 at 03:34:42PM -0700, Song Liu wrote:
> Huge page backed vmalloc memory could benefit performance in many cases.
> Since some users of vmalloc may not be ready to handle huge pages,
> VM_NO_HUGE_VMAP was introduced to allow vmalloc users to opt-out huge
> pages. However, it is not easy to add VM_NO_HUGE_VMAP to all the users
> that may try to allocate >= PMD_SIZE pages, but are not ready to handle
> huge pages properly.
> 
> Replace VM_NO_HUGE_VMAP with an opt-in flag, VM_ALLOW_HUGE_VMAP, so that
> users that benefit from huge pages could ask specificially.

Given that the huge page backing was added explicitly for some big boot
time allocated hashed,those should probably have the VM_ALLOW_HUGE_VMAP
added from the start (maybe not in this patch, but certainly in this
series).  We'll probably also need a vmalloc_huge interface for those.

Re: [PATCH bpf 2/2] bpf: use vmalloc with VM_ALLOW_HUGE_VMAP for bpf_prog_pack

[Christoph Hellwig]

On Fri, Apr 08, 2022 at 03:34:43PM -0700, Song Liu wrote:
> +static void *bpf_prog_pack_vmalloc(unsigned long size)
> +{
> +#if defined(MODULES_VADDR)
> +	unsigned long start = MODULES_VADDR;
> +	unsigned long end = MODULES_END;
> +#else
> +	unsigned long start = VMALLOC_START;
> +	unsigned long end = VMALLOC_END;
> +#endif
> +
> +	return __vmalloc_node_range(size, PAGE_SIZE, start, end, GFP_KERNEL, PAGE_KERNEL,
> +				    VM_DEFER_KMEMLEAK | VM_ALLOW_HUGE_VMAP,
> +				    NUMA_NO_NODE, __builtin_return_address(0));
> +}

Instead of having this magic in bpf I think a module_alloc_large would
seems like the better interface here.

Re: [PATCH bpf 0/2] vmalloc: bpf: introduce VM_ALLOW_HUGE_VMAP

[Thorsten Leemhuis]

Hi, this is your Linux kernel regression tracker.

On 09.04.22 00:34, Song Liu wrote:
> Enabling HAVE_ARCH_HUGE_VMALLOC on x86_64 and use it for bpf_prog_pack has
> caused some issues [1], as many users of vmalloc are not yet ready to
> handle huge pages. To enable a more smooth transition to use huge page
> backed vmalloc memory, this set replaces VM_NO_HUGE_VMAP flag with an new
> opt-in flag, VM_ALLOW_HUGE_VMAP. More discussions about this topic can be
> found at [2].
> 
> Patch 1 removes VM_NO_HUGE_VMAP and adds VM_ALLOW_HUGE_VMAP.
> Patch 2 uses VM_ALLOW_HUGE_VMAP in bpf_prog_pack.
> 
> [1] https://lore.kernel.org/lkml/20220204185742.271030-1-song@kernel.org/
> [2] https://lore.kernel.org/linux-mm/20220330225642.1163897-1-song@kernel.org/

These patches apparently fix a regression (one that's mentioned in your
[2]) that I tracked. Hence in the next iteration of your patches could
you please instead add a 'Link:' tag pointing to the report for anyone
wanting to look into the backstory in the future, as explained in
'Documentation/process/submitting-patches.rst' and
'Documentation/process/5.Posting.rst'? E.g. like this:

"Link:
https://lore.kernel.org/netdev/14444103-d51b-0fb3-ee63-c3f182f0b546@molgen.mpg.de/"

Not totally sure, but I guess it needs a Fixes tag as well specifying
the change that cause this regression (that's "fac54e2bfb5b"). The
documents mentioned above explain this, too. A "Reported-by" might be
appropriate as well.

In anyone wonders why I care: there are internal and publicly used tools
and scripts out there that reply on proper "Link" tags. I don't known
how many, but there is at least one public tool I'm running that cares:
regzbot, my regression tracking bot, which I use to track Linux kernel
regressions and generate the regression reports sent to Linus. Proper
"Link:" tags allow the bot to automatically connect regression reports
with fixes being posted or applied to resolve the particular regression
-- which makes regression tracking a whole lot easier and feasible for
the Linux kernel. That's why it's a great help for me if people set
proper "Link" tags.

While at it, let me tell regzbot about this thread:
#regzbot ^backmonitor:
https://lore.kernel.org/netdev/14444103-d51b-0fb3-ee63-c3f182f0b546@molgen.mpg.de/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I'm getting a lot of
reports on my table. I can only look briefly into most of them and lack
knowledge about most of the areas they concern. I thus unfortunately
will sometimes get things wrong or miss something important. I hope
that's not the case here; if you think it is, don't hesitate to tell me
in a public reply, it's in everyone's interest to set the public record
straight.

Re: [PATCH bpf 1/2] vmalloc: replace VM_NO_HUGE_VMAP with VM_ALLOW_HUGE_VMAP

[Song Liu]

> On Apr 8, 2022, at 10:28 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Fri, Apr 08, 2022 at 03:34:42PM -0700, Song Liu wrote:
>> Huge page backed vmalloc memory could benefit performance in many cases.
>> Since some users of vmalloc may not be ready to handle huge pages,
>> VM_NO_HUGE_VMAP was introduced to allow vmalloc users to opt-out huge
>> pages. However, it is not easy to add VM_NO_HUGE_VMAP to all the users
>> that may try to allocate >= PMD_SIZE pages, but are not ready to handle
>> huge pages properly.
>> 
>> Replace VM_NO_HUGE_VMAP with an opt-in flag, VM_ALLOW_HUGE_VMAP, so that
>> users that benefit from huge pages could ask specificially.
> 
> Given that the huge page backing was added explicitly for some big boot
> time allocated hashed,those should probably have the VM_ALLOW_HUGE_VMAP
> added from the start (maybe not in this patch, but certainly in this
> series).  We'll probably also need a vmalloc_huge interface for those.

Will add it in v2. 

Thanks,
Song

Re: [PATCH bpf 2/2] bpf: use vmalloc with VM_ALLOW_HUGE_VMAP for bpf_prog_pack

[Song Liu]

> On Apr 8, 2022, at 10:29 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Fri, Apr 08, 2022 at 03:34:43PM -0700, Song Liu wrote:
>> +static void *bpf_prog_pack_vmalloc(unsigned long size)
>> +{
>> +#if defined(MODULES_VADDR)
>> +	unsigned long start = MODULES_VADDR;
>> +	unsigned long end = MODULES_END;
>> +#else
>> +	unsigned long start = VMALLOC_START;
>> +	unsigned long end = VMALLOC_END;
>> +#endif
>> +
>> +	return __vmalloc_node_range(size, PAGE_SIZE, start, end, GFP_KERNEL, PAGE_KERNEL,
>> +				    VM_DEFER_KMEMLEAK | VM_ALLOW_HUGE_VMAP,
>> +				    NUMA_NO_NODE, __builtin_return_address(0));
>> +}
> 
> Instead of having this magic in bpf I think a module_alloc_large would
> seems like the better interface here.

AFAICT, modules allocate a large piece of memory and put both text and
data on it, so modules cannot really use huge pages yet. 

OTOH, it is probably beneficial for the modules to use something 
similar to bpf_prog_pack, i.e., put text from multiple modules to a 
single huge page. Of course, this requires non-trivial work in both 
mm code and module code.

Given that 1) modules cannot use huge pages yet, and 2) module may
use differently (with sharing), I think adding module_alloc_large()
doesn't add much value at the moment. So we can just keep this logic
in BPF for now. 

Does this make sense?

Thanks,
Song

Re: [PATCH bpf 0/2] vmalloc: bpf: introduce VM_ALLOW_HUGE_VMAP

[Song Liu]

> On Apr 9, 2022, at 4:43 AM, Thorsten Leemhuis <regressions@leemhuis.info> wrote:
> 
> Hi, this is your Linux kernel regression tracker.
> 
> On 09.04.22 00:34, Song Liu wrote:
>> Enabling HAVE_ARCH_HUGE_VMALLOC on x86_64 and use it for bpf_prog_pack has
>> caused some issues [1], as many users of vmalloc are not yet ready to
>> handle huge pages. To enable a more smooth transition to use huge page
>> backed vmalloc memory, this set replaces VM_NO_HUGE_VMAP flag with an new
>> opt-in flag, VM_ALLOW_HUGE_VMAP. More discussions about this topic can be
>> found at [2].
>> 
>> Patch 1 removes VM_NO_HUGE_VMAP and adds VM_ALLOW_HUGE_VMAP.
>> Patch 2 uses VM_ALLOW_HUGE_VMAP in bpf_prog_pack.
>> 
>> [1] https://lore.kernel.org/lkml/20220204185742.271030-1-song@kernel.org/
>> [2] https://lore.kernel.org/linux-mm/20220330225642.1163897-1-song@kernel.org/
> 
> These patches apparently fix a regression (one that's mentioned in your
> [2]) that I tracked. Hence in the next iteration of your patches could
> you please instead add a 'Link:' tag pointing to the report for anyone
> wanting to look into the backstory in the future, as explained in
> 'Documentation/process/submitting-patches.rst' and
> 'Documentation/process/5.Posting.rst'? E.g. like this:
> 
> "Link:
> https://lore.kernel.org/netdev/14444103-d51b-0fb3-ee63-c3f182f0b546@molgen.mpg.de/"
> 
> Not totally sure, but I guess it needs a Fixes tag as well specifying
> the change that cause this regression (that's "fac54e2bfb5b"). The
> documents mentioned above explain this, too. A "Reported-by" might be
> appropriate as well.
> 
> In anyone wonders why I care: there are internal and publicly used tools
> and scripts out there that reply on proper "Link" tags. I don't known
> how many, but there is at least one public tool I'm running that cares:
> regzbot, my regression tracking bot, which I use to track Linux kernel
> regressions and generate the regression reports sent to Linus. Proper
> "Link:" tags allow the bot to automatically connect regression reports
> with fixes being posted or applied to resolve the particular regression
> -- which makes regression tracking a whole lot easier and feasible for
> the Linux kernel. That's why it's a great help for me if people set
> proper "Link" tags.
> 
> While at it, let me tell regzbot about this thread:
> #regzbot ^backmonitor:
> https://lore.kernel.org/netdev/14444103-d51b-0fb3-ee63-c3f182f0b546@molgen.mpg.de/
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> 
> P.S.: As the Linux kernel's regression tracker I'm getting a lot of
> reports on my table. I can only look briefly into most of them and lack
> knowledge about most of the areas they concern. I thus unfortunately
> will sometimes get things wrong or miss something important. I hope
> that's not the case here; if you think it is, don't hesitate to tell me
> in a public reply, it's in everyone's interest to set the public record
> straight.

Thanks for the reminder. I will add the Fixes tag, and try to work with 
regzbot. 

Song

Re: [PATCH bpf 2/2] bpf: use vmalloc with VM_ALLOW_HUGE_VMAP for bpf_prog_pack

[Christoph Hellwig]

On Sun, Apr 10, 2022 at 01:34:50AM +0000, Song Liu wrote:
> OTOH, it is probably beneficial for the modules to use something 
> similar to bpf_prog_pack, i.e., put text from multiple modules to a 
> single huge page. Of course, this requires non-trivial work in both 
> mm code and module code.
> 
> Given that 1) modules cannot use huge pages yet, and 2) module may
> use differently (with sharing), I think adding module_alloc_large()
> doesn't add much value at the moment. So we can just keep this logic
> in BPF for now. 
> 
> Does this make sense?

I'm not intending to say modules should use the new helper.  But I'd much
prefer to keep all the MODULES_VADDR related bits self-contained in the
modules code and not splatter it over random other subsystems.

Re: [PATCH bpf 2/2] bpf: use vmalloc with VM_ALLOW_HUGE_VMAP for bpf_prog_pack

[Song Liu]

> On Apr 10, 2022, at 11:56 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Sun, Apr 10, 2022 at 01:34:50AM +0000, Song Liu wrote:
>> OTOH, it is probably beneficial for the modules to use something 
>> similar to bpf_prog_pack, i.e., put text from multiple modules to a 
>> single huge page. Of course, this requires non-trivial work in both 
>> mm code and module code.
>> 
>> Given that 1) modules cannot use huge pages yet, and 2) module may
>> use differently (with sharing), I think adding module_alloc_large()
>> doesn't add much value at the moment. So we can just keep this logic
>> in BPF for now. 
>> 
>> Does this make sense?
> 
> I'm not intending to say modules should use the new helper.  But I'd much
> prefer to keep all the MODULES_VADDR related bits self-contained in the
> modules code and not splatter it over random other subsystems.

Got it. Will add that in v2. 

Thanks,
Song