Guenter Roeck, Any chance you can give this branch a good spin on your multi-arch setup to see what may below up? 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: > Hi Luis, > > Thanks for your kind review! > > On Wed, Jan 25, 2023 at 10:06 PM Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote: > > > [...] > > > > > > 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. > > 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"). > > > Signed-off-by: Song Liu <song@xxxxxxxxxx> > > > Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx> > > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx> > > > > > > --- > > > > > > 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. > > > 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@xxxxxxxxxx/T/#u > > > > 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
