On Tue, Aug 18, 2020 at 07:30:33PM +0300, Jarkko Sakkinen wrote: > On Tue, Aug 18, 2020 at 02:51:41PM +0300, Mike Rapoport wrote: > > On Tue, Aug 18, 2020 at 08:30:29AM +0300, Jarkko Sakkinen wrote: > > > On Sun, Jul 26, 2020 at 11:14:08AM +0300, Mike Rapoport wrote: > > > > > > > > > > I'm not still sure that I fully understand this feedback as I don't see > > > > > any inherent and obvious difference to the v4. In that version fallbacks > > > > > are to module_alloc() and module_memfree() and text_alloc() and > > > > > text_memfree() can be overridden by arch. > > > > > > > > The major difference between your v4 and my suggestion is that I'm not > > > > trying to impose a single ARCH_HAS_TEXT_ALLOC as an alternative to > > > > MODULES but rather to use per subsystem config option, e.g. > > > > HAVE_KPROBES_TEXT_ALLOC. > > > > > > > > Another thing, which might be worth doing regardless of the outcome of > > > > this discussion is to rename alloc_insn_pages() to text_alloc_kprobes() > > > > because the former is way too generic and does not emphasize that the > > > > instruction page is actually used by kprobes only. > > > > > > What if we in kernel/kprobes.c just: > > > > > > #ifndef CONFIG_ARCH_HAS_TEXT_ALLOC > > > > I don't think that CONFIG_ARCH_HAS_TEXT_ALLOC will work for all > > architectures. > > > > If an architecture has different restrictions for allocation of text > > for, say, modules, kprobes, bfp, it won't be able to use a single > > ARCH_HAS_TEXT_ALLOC. Which means that this architecture is stuck with > > dependency of kprobes on MODULES until the next rework. > > I was thinking to name it as CONFIG_HAS_KPROBES_ALLOC_PAGE, alogn the > lines described below, so it is merely a glitch in my example. IMHO, it would be better to emphasize that this is text page. I liked Mark's idea [1] to have text_alloc_<subsys>() naming for the allocation functions. The Kconfig options then would become HAVE_TEXT_ALLOC_<SUBSYS>. [1] https://lore.kernel.org/linux-riscv/20200714133314.GA67386@C02TD0UTHF1T.local/ > > > void __weak *alloc_insn_page(void) > > > { > > > return module_alloc(PAGE_SIZE); > > > } > > > > > > void __weak free_insn_page(void *page) > > > { > > > module_memfree(page); > > > } > > > #endif > > > > > > In Kconfig (as in v5): > > > > > > config KPROBES > > > bool "Kprobes" > > > depends on MODULES || ARCH_HAS_TEXT_ALLOC > > > > > > I checked architectures that override alloc_insn_page(). With the > > > exception of x86, they do not call module_alloc(). > > > > > > If no rename was done, then with this approach a more consistent. > > > config flag name would be CONFIG_ARCH_HAS_ALLOC_INSN_PAGE. > > > > > > I'd call the function just as kprobes_alloc_page(). Then the > > > config flag would become CONFIG_HAS_KPROBES_ALLOC_PAGE. > > > > > > > -- > > > > Sincerely yours, > > > > Mike. > > > > > > Thanks for the feedback! > > > > > > /Jarkko > > > > > -- > > Sincerely yours, > > Mike. > > BR, > /Jarkko -- Sincerely yours, Mike.
