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: > > 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
