On Thu, Jan 26, 2023 at 01:00:43PM -0800, Luis Chamberlain wrote:

Guenter Roeck,

Any chance you can give this branch a good spin on your multi-arch setup
to see what may below up?

I assume I shoud test v3 ?

Guenter

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=modules-testing

On Thu, Jan 26, 2023 at 12:01:40PM -0800, Song Liu wrote:

quoted

Hi Luis,

Thanks for your kind review!

On Wed, Jan 25, 2023 at 10:06 PM Luis Chamberlain [off-list ref] wrote:

quoted

[...]

quoted

quoted

        MOD_MEM_TYPE_TEXT,
        MOD_MEM_TYPE_DATA,
        MOD_MEM_TYPE_RODATA,
        MOD_MEM_TYPE_RO_AFTER_INIT,
        MOD_MEM_TYPE_INIT_TEXT,
        MOD_MEM_TYPE_INIT_DATA,
        MOD_MEM_TYPE_INIT_RODATA,

and allocating them separately.

First thanks for doing this work!

This seems to not acknolwedge the original goal of the first module_layout and
the latched rb-tree use, and that was was for speeding up __module_address()
since it *can* even be triggered on NMIs. I say this because the first question
that comes to me is the impact to performance on __module_address() I can't
see that changing much here, but mention it as it similar consideration
should be made in case future changes modify this path.

To make sure I understand this correctly. Do you mean we need something like
the following in the commit log?

"""
This adds slightly more entries to mod_tree (from up to 3 entries per module, to
up to 7 entries per module). However, this at most adds a small constant
overhead to __module_address(), which is expected to be fast.
"""

Yes I think this is very useful information for the reviewier.

quoted

quoted

Microbenching something so trivial as __module_address() may not be as useful
for an idle system, at the very least being able to compare before and after
even on idle may be useful *if* you eventually do some more radical changes
here. Modules-related kernel/kallsyms_selftest.c did that for kallsyms_lookup_name()
and friend just recently for a minor performance enhancement.

kernel/kallsyms_selftest.c is new to me. I will give it a try.

It was just merged, be sure to have da35048f2600 ("kallsyms: Fix
scheduling with interrupts disabled in self-test").

quoted

quoted

quoted

Signed-off-by: Song Liu <song@kernel.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Thomas Gleixner <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>

---

This is the preparation work for the type aware module_alloc() discussed
in [1]. While this work is not covered much in the discussion, it is a
critical step of the effort.

As this part grows pretty big (~1000 lines, + and -), I would like get
some feedback on it, so that I know it is on the right track.

Please share your comments. Thanks!

Test coverage: Tested on x86_64.

I will likely merge this onto modules-next soon, not because I think it is
ready, but just because I think it *is* mostly ready and the next thing
we need is exposure and testing. rc5 is pretty late to consider this
for v6.3 and so hopefully for this cycle we can at least settle on
something which will sit in linux-next since the respective linux-next
after v6.3-rc1 is released.

Yes, this definitely needs more tests. Given different archs use
module_layout in all sorts of ways. I will be very surprised if I updated
all them correctly (though I tried hard to).

OK so let's be patient with testing. Getting help from Guenter here
can probably speed up finding issues.

quoted

quoted

quoted

Build tested by kernel test bot in [2]. The only regression in [2] was a
typo in parisc, which is also fixed.

[1] https://lore.kernel.org/linux-mm/20221107223921.3451913-1-song@kernel.org/T/#u (local)

You still never addressed my performance suggestions so don't be
surprised if I insist later. Yes you can use existing performance
benchmarks, specially now with modules as a hard requirement, to
show gains. So I'd like to clarify that if I'm reviewing late it is
because:

a) my modules patch review queue has been high as of late
b) you seem to not have taken these performance suggestions into consideration
   before and so I tend to put it at my end of my queue for review.

I think it will be a lot easier to run performance tests with the
module support. Let's see what we can do when we get to the
performance test part.

Fantastic, clearly I'm interested in being able to reproduce so I will
email you offline about some techniques I've used to reproduce some
things easily for testing things with modules.

  Luis