Re: [PATCH RFC tip/core/rcu 0/4] Forbid static SRCU use in modules

On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote: > On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote: > > > > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google joel(a)joelfernandes.org > > wrote: > > > > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote: > > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck paulmck(a)linux.ibm.com wrote: > > >> > > >> > On Sun, Apr 07, 2019 at 06:39:41AM -0700, Paul E. McKenney wrote: > > >> >> On Sat, Apr 06, 2019 at 07:06:13PM -0400, Joel Fernandes wrote: > > >> > > > >> > [ . . . ] > > >> > > > >> >> > > diff --git a/include/asm-generic/vmlinux.lds.h > > >> >> > > b/include/asm-generic/vmlinux.lds.h > > >> >> > > index f8f6f04c4453..c2d919a1566e 100644 > > >> >> > > --- a/include/asm-generic/vmlinux.lds.h > > >> >> > > +++ b/include/asm-generic/vmlinux.lds.h > > >> >> > > @@ -338,6 +338,10 @@ > > >> >> > > KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \ > > >> >> > > __stop___tracepoints_ptrs = .; \ > > >> >> > > *(__tracepoints_strings)/* Tracepoints: strings */ \ > > >> >> > > + . = ALIGN(8); \ > > >> >> > > + __start___srcu_struct = .; \ > > >> >> > > + *(___srcu_struct_ptrs) \ > > >> >> > > + __end___srcu_struct = .; \ > > >> >> > > } \ > > >> >> > > > >> >> > This vmlinux linker modification is not needed. I tested without it and srcu > > >> >> > torture works fine with rcutorture built as a module. Putting further prints > > >> >> > in kernel/module.c verified that the kernel is able to find the srcu structs > > >> >> > just fine. You could squash the below patch into this one or apply it on top > > >> >> > of the dev branch. > > >> >> > > >> >> Good point, given that otherwise FORTRAN named common blocks would not > > >> >> work. > > >> >> > > >> >> But isn't one advantage of leaving that stuff in the RO_DATA_SECTION() > > >> >> macro that it can be mapped read-only? Or am I suffering from excessive > > >> >> optimism? > > >> > > > >> > And to answer the other question, in the case where I am suffering from > > >> > excessive optimism, it should be a separate commit. Please see below > > >> > for the updated original commit thus far. > > >> > > > >> > And may I have your Tested-by? > > >> > > >> Just to confirm: does the cleanup performed in the modules going > > >> notifier end up acting as a barrier first before freeing the memory ? > > >> If not, is it explicitly stated that a barrier must be issued before > > >> module unload ? > > >> > > > > > > You mean rcu_barrier? It is mentioned in the documentation that this is the > > > responsibility of the module writer to prevent delays for all modules. > > > > It's a srcu barrier yes. Considering it would be a barrier specific to the > > srcu domain within that module, I don't see how it would cause delays for > > "all" modules if we implicitly issue the barrier on module unload. What > > am I missing ? > > Yes you are right. I thought of this after I just sent my email. I think it > makes sense for srcu case to do and could avoid a class of bugs. If there are call_srcu() callbacks outstanding, the module writer still needs the srcu_barrier() because otherwise callbacks arrive after the module text has gone, which will be disappoint the CPU when it tries fetching instructions that are no longer mapped. If there are no call_srcu() callbacks from that module, then there is no need for srcu_barrier() either way. So if an srcu_barrier() is needed, the module developer needs to supply it.