Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230526051529.3387103-1-song@kernel.org> <20230526051529.3387103-2-song@kernel.org>
 <ZHEy5SkFwI+dZjOC@moria.home.lan> <CAPhsuW5nbXozvBs1X7q_u=eTY0U=Ep32n1bM8bxR6h9UEU3AxQ@mail.gmail.com>
 <ZHFDO/95KMXRdOWI@moria.home.lan> <CAPhsuW5CTANR_B57w5n3HvdDjvHXOCSGTDc=a4s+JR0pKCAOcw@mail.gmail.com>
 <ZHF21CSxyjQy9C7F@moria.home.lan>
In-Reply-To: <ZHF21CSxyjQy9C7F@moria.home.lan>
From:   Song Liu <song@kernel.org>
Date:   Fri, 26 May 2023 23:00:27 -0700
Message-ID: <CAPhsuW5dx_A_brhf0HHWMWj3HB_31QCQ8xbqZPir8YxHM5WRig@mail.gmail.com>
Subject: Re: [PATCH 1/3] module: Introduce module_alloc_type
To:     Kent Overstreet <kent.overstreet@linux.dev>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, May 26, 2023 at 8:20=E2=80=AFPM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
[...]
> > > void jitalloc_update(void *dst, void *src, size_t len)
> > > {
> > >         if (text_poke_available) {
> > >                 text_poke(...);
> > >         } else {
> > >                 unprotect();
> > >                 memcpy();
> > >                 protect();
> > >         }
> > > }
> >
> > I think this doesn't work for sub page allocation?
>
> Perhaps I elided too much - it does if you add a single lock. You can't
> do that if it's not a common helper.

Actually, sub page allocation is not the problem. The problem is
with unprotect() and protect(), as they require global TLB flush.

>
> > At the end of all this, we will have modules running from huge pages, d=
ata
> > and text. It will give significant performance benefit when some key dr=
iver
> > cannot be compiled into the kernel.
>
> Yeah, I've seen the numbers for the perf impact of running as a module
> due to the extra TLB overhead - but Mike's recent data was showing that
> this doesn't matter nearly as much as data as it does for text.
>
> > > Also - module_memory_fill_type(), module_memory_invalidate_type() - I
> > > know these are for BPF, but could you explain why we need them in the
> > > external interface here? Could they perhaps be small helpers in the b=
pf
> > > code that use something like jitalloc_update()?
> >
> > These are used by all users, not just BPF. 1/3 uses them in
> > kernel/module/main.c. I didn't use them in 3/3 as it is arch code, but =
I can
> > use them instead of text_poke_* (and that is probably better code style=
).
> > As I am writing the email, I think I should use it in ftrace.c (2/3) to=
 avoid
> > the __weak function.
>
> Ok. Could we make it clearer why they're needed and what they're for?
>
> I know bpf fills in invalid instructions initially; I didn't read
> through enough code to find the "why", and let's document that to save
> other people the same effort.
>
> And do they really need to be callbacks in mod_alloc_params...?

If we want to call them from common code, they will either be callback
or some __weak functions.

>
> Do we need the other things in mod_alloc_params/vmalloc_params?
>  - your granularity field says it's for specifying PAGE/PMD size: we
>    definitely do not want that. We've had way too much code along the
>    lines of "implement hugepages for x", we need to stop doing that.
>    It's an internal mm/ detail.

We don't need "granularity" yet. We will need them with the binpack
allocator.

>  - vmalloc_params: we need gfp_t and pgprot_t, but those should be
>    normal arguments. start/end/vm_flags? They seem like historical
>    module baggage, can we get rid of them?

All these fields are needed by some of the module_alloc()s for different
archs. I am not an expert of all the archs, so I cannot say whether some
of them are not needed.
[...]
> > I don't really think module code is very messy at the moment. If
> > necessary, we can put module_alloc_type related code in a
> > separate file.
>
> Hey, it's been organized since I last looked at it :) But I tihink this
> belongs in mm/, not module code.

I don't have a strong preference for this (at the moment). If we agree
it belongs to mm/, we sure can move it.

Thanks,
Song