> On Jul 11, 2022, at 9:18 PM, Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote: > > On Sat, Jul 09, 2022 at 01:14:23AM +0000, Song Liu wrote: >>> On Jul 8, 2022, at 3:24 PM, Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote: >>> >>>> 1) Rename module_alloc_huge as module_alloc_text_huge(); >>> >>> module_alloc_text_huge() is too long, but I've suggested names before >>> which are short and generic, and also suggested that if modules are >>> not the only users this needs to go outside of modules and so >>> vmalloc_text_huge() or whatever. >>> >>> To do this right it begs the question why we don't do that for the >>> existing module_alloc(), as the users of this code is well outside of >>> modules now. Last time a similar generic name was used all the special >>> arch stuff was left to be done by the module code still, but still >>> non-modules were still using that allocator. From my perspective the >>> right thing to do is to deal with all the arch stuff as well in the >>> generic handler, and have the module code *and* the other users which >>> use module_alloc() to use that new caller as well. >> >> The key difference between module_alloc() and the new API is that the >> API will return RO+X memory, and the user need text-poke like API to >> modify this buffer. Archs that do not support text-poke will not be >> able to use the new API. Does this sound like a reasonable design? [...] > I believe you are mentioning requiring text_poke() because the way > eBPF code uses the module_alloc() is different. Correct me if I'm > wrong, but from what I gather is you use the text_poke_copy() as the data > is already RO+X, contrary module_alloc() use cases. You do this since your > bpf_prog_pack_alloc() calls set_memory_ro() and set_memory_x() after > module_alloc() and before you can use this memory. This is a different type > of allocator. And, again please correct me if I'm wrong but now you want to > share *one* 2 MiB huge-page for multiple BPF programs to help with the > impact of TLB misses. Yes, sharing 1x 2MiB huge page is the main reason to require text_poke. OTOH, 2MiB huge pages without sharing is not really useful. Both kprobe and ftrace only uses a fraction of a 4kB page. Most BPF programs and modules cannot use 2MiB either. Therefore, vmalloc_rw_exec() doesn't add much value on top of current module_alloc(). > A vmalloc_ro_exec() by definition would imply a text_poke(). > > Can kprobes, ftrace and modules use it too? It would be nice > so to not have to deal with the loose semantics on the user to > have to use set_vm_flush_reset_perms() on ro+x later, but > I think this can be addressed separately on a case by case basis. I am pretty confident that kprobe and ftrace can share huge pages with BPF programs. I haven't looked into all the details with modules, but given CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC, I think it is also possible. Once this is done, a regular system (without huge BPF program or huge modules) will just use 1x 2MB page for text from module, ftrace, kprobe, and bpf programs. > > But a vmalloc_ro_exec() with a respective free can remove the > requirement to do set_vm_flush_reset_perms(). Removing the requirement to set_vm_flush_reset_perms() is the other reason to go directly to vmalloc_ro_exec(). My current version looks like this: void *vmalloc_exec(unsigned long size); void vfree_exec(void *ptr, unsigned int size); ro is eliminated as there is no rw version of the API. The ugly part is @size for vfree_exec(). We need it to share huge pages. Under the hood, it looks similar to current bpf_prog_pack_alloc and bpf_prog_pack_free. Thanks, Song
