Re: [PATCH 0/3] Type aware module allocator

From: Kent Overstreet <kent.overstreet@linux.dev>
To: Song Liu <song@kernel.org>
Cc: linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
	mcgrof@kernel.org, peterz@infradead.org, tglx@linutronix.de,
	x86@kernel.org, rppt@kernel.org
Subject: Re: [PATCH 0/3] Type aware module allocator
Date: Thu, 1 Jun 2023 14:21:46 -0400	[thread overview]
Message-ID: <ZHjhuju75wGR/AT2@moria.home.lan> (raw)
In-Reply-To: <CAPhsuW6hqzLuNhvkHFOmKTJdQm8A0JdUna=1iFdRC0y+kKmF4Q@mail.gmail.com>

On Tue, May 30, 2023 at 03:48:51PM -0700, Song Liu wrote:
> On Mon, May 29, 2023 at 11:25 AM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> >
> > On Sat, May 27, 2023 at 10:58:37PM -0700, Song Liu wrote:
> > > I don't think we are exposing architecture specific options to users.
> > > Some layer need to handle arch specifics. If the new allocator is
> > > built on top of module_alloc, module_alloc is handling that. If the new
> > > allocator is to replace module_alloc, it needs to handle arch specifics.
> >
> > Ok, I went back and read more thoroughly, I got this part wrong. The
> > actual interface is the mod_mem_type enum, not mod_alloc_params or
> > vmalloc_params.
> >
> > So this was my main complaint, but this actually looks ok now.
> >
> > It would be better to have those structs in a .c file, not the header
> > file - it looks like those are the public interface the way you have it.
> 
> Thanks for this suggestion. It makes a lot of sense. But I am not quite
> sure how we can avoid putting it in the header yet. I will take a closer
> look. OTOH, if we plan to use Mike's new allocator to replace vmalloc,
> we probably don't need this part.

Well, right now module_alloc() uses arch-exported constants, we could
keep doing that if it makes sense.

Or the structs could go in a separate header file that better indicates
what they're for. The main point is just that - when we're writing new
code and creating a new interface, it's very helpful if we can have the
header file basically be the documentation for what the external
interface is. Put your big kernel doc "what is this thing about" comment
in that file, too :)

> > We just need a helper that either calls text_poke() or does the page
> > permission dance in a single place.
> 
> AFAICT, we don't have a global text_poke() API yet. I can take a look
> into it (if it makes sense).

I don't think anyone's working on this yet, so if you're interested I
think it might be helpful.

> > If we do the same thing for other mod_mem_types, we could potentially
> > allow them to be shared on hugepages too.
> 
> Yeah, that's part of the goal to extend the scope from executable to all
> types.

Yeah, I think the "enumerate types of allocations that want similar page
permission handling" is a good thing to focus on.