Re: [PATCH v5 5/6] kprobes: Use text_alloc() and text_free()

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Wed, Aug 19, 2020 at 09:47:18AM +0300, Mike Rapoport wrote:
> 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/

I think I got the point now, the point being in future other subsystems
could use the same naming convention for analogous stuff?

I'll follow this convention. Thank you for the patience with this!

/Jarkko