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

[Joel Fernandes]

On Tue, Apr 09, 2019 at 11:56:03AM -0400, Mathieu Desnoyers wrote:
> - On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google 
> j...@joelfernandes.org wrote:
> 
> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> >> - On Apr 8, 2019, at 11:46 AM, paulmck paul...@linux.ibm.com wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> >> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:
> >> >> 
> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com 
> >> >> >> wrote:
> >> >> >> 
> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
> >> >> >> >> > j...@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 
> >> >> >> >> > >> paul...@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

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

[Mathieu Desnoyers]

- On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google 
j...@joelfernandes.org wrote:

> On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
>> - On Apr 8, 2019, at 11:46 AM, paulmck paul...@linux.ibm.com wrote:
>> 
>> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
>> >> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:
>> >> 
>> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> >> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com wrote:
>> >> >> 
>> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
>> >> >> >> > j...@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 
>> >> >> >> > >> paul...@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 
>

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

[Mathieu Desnoyers]

- On Apr 9, 2019, at 12:40 PM, paulmck paul...@linux.ibm.com wrote:

> On Tue, Apr 09, 2019 at 11:56:03AM -0400, Mathieu Desnoyers wrote:
>> - On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google 
>> j...@joelfernandes.org
>> wrote:
>> 
>> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
>> >> - On Apr 8, 2019, at 11:46 AM, paulmck paul...@linux.ibm.com wrote:
>> >> 
>> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
>> >> >> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:
>> >> >> 
>> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> >> >> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com 
>> >> >> >> wrote:
>> >> >> >> 
>> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
>> >> >> >> >> > j...@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 
>> >> >> >> >> > >> paul...@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 
>> >> >>

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

[Joel Fernandes]

On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> - On Apr 8, 2019, at 11:46 AM, paulmck paul...@linux.ibm.com wrote:
> 
> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com wrote:
> >> >> 
> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
> >> >> >> > j...@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 paul...@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 ca

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

[Paul E. McKenney]

On Tue, Apr 09, 2019 at 02:04:11PM -0400, Mathieu Desnoyers wrote:
> - On Apr 9, 2019, at 1:55 PM, paulmck paul...@linux.ibm.com wrote:
> [...]
> > The current state is not horrible, so my thought would be to give it
> > some time to see if better thoughts arise.
> > 
> > Either way, cleanup_srcu_struct() keeps its current checks for callbacks
> > still being in flight, which is why I believe that the current state is
> > not horrible.  ;-)
> 
> In that case, I think the comment above cleanup_srcu_struct_quiesced() in
> include/linux/srcu.h needs to be updated to cover situations where API
> users statically define a SRCU domain in a module and intend to unload
> that module.
> 
> Given that we end up doing the allocation/cleanup under the hood, the
> API users don't interact with init_srcu_struct() nor cleanup_srcu_struct(),
> so it's not obvious that this comment also applies to them.

Actually, it turned out that cleanup_srcu_struct_quiesced() is extremely
hard to use correctly, and maybe even impossible to use correctly.
So cleanup_srcu_struct_quiesced has been eliminated in current -rcu.

Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul E. McKenney]

On Tue, Apr 09, 2019 at 11:56:03AM -0400, Mathieu Desnoyers wrote:
> - On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google 
> j...@joelfernandes.org wrote:
> 
> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> >> - On Apr 8, 2019, at 11:46 AM, paulmck paul...@linux.ibm.com wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> >> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:
> >> >> 
> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com 
> >> >> >> wrote:
> >> >> >> 
> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
> >> >> >> >> > j...@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 
> >> >> >> >> > >> paul...@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

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

[Paul E. McKenney]

On Tue, Apr 09, 2019 at 12:45:25PM -0400, Mathieu Desnoyers wrote:
> - On Apr 9, 2019, at 12:40 PM, paulmck paul...@linux.ibm.com wrote:
> 
> > On Tue, Apr 09, 2019 at 11:56:03AM -0400, Mathieu Desnoyers wrote:
> >> - On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google 
> >> j...@joelfernandes.org
> >> wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> >> >> - On Apr 8, 2019, at 11:46 AM, paulmck paul...@linux.ibm.com wrote:
> >> >> 
> >> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> >> >> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com 
> >> >> >> wrote:
> >> >> >> 
> >> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com 
> >> >> >> >> wrote:
> >> >> >> >> 
> >> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
> >> >> >> >> >> > j...@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 
> >> >> >> >> >> > >> paul...@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 no

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

[Mathieu Desnoyers]

- On Apr 9, 2019, at 1:55 PM, paulmck paul...@linux.ibm.com wrote:
[...]
> The current state is not horrible, so my thought would be to give it
> some time to see if better thoughts arise.
> 
> Either way, cleanup_srcu_struct() keeps its current checks for callbacks
> still being in flight, which is why I believe that the current state is
> not horrible.  ;-)

In that case, I think the comment above cleanup_srcu_struct_quiesced() in
include/linux/srcu.h needs to be updated to cover situations where API
users statically define a SRCU domain in a module and intend to unload
that module.

Given that we end up doing the allocation/cleanup under the hood, the
API users don't interact with init_srcu_struct() nor cleanup_srcu_struct(),
so it's not obvious that this comment also applies to them.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mathieu Desnoyers]

- On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:

> On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com wrote:
>> 
>> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
>> >> > j...@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 paul...@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.
>> 
>> When you say "callbacks arrive after the module text has gone",
>> I think you assume that free_module() is invoked before the
>> MODULE_STATE_GOING notifiers are called. But it's done in the
>> opposite order: going notifiers are called first, and then
>> free_module() 

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

[Mathieu Desnoyers]

- On Apr 8, 2019, at 11:46 AM, paulmck paul...@linux.ibm.com wrote:

> On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
>> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:
>> 
>> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com wrote:
>> >> 
>> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
>> >> >> > j...@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 paul...@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 ther

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

[Paul E. McKenney]

On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> - On Apr 8, 2019, at 10:22 AM, paulmck paul...@linux.ibm.com wrote:
> 
> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com wrote:
> >> 
> >> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
> >> >> > j...@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 paul...@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 develope

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

[Paul E. McKenney]

On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> - On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com wrote:
> 
> > On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
> >> > j...@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 paul...@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.
> 
> When you say "callbacks arrive after the module text has gone",
> I think you assume that free_module() is invoked before the
> MODULE_STATE_GOING notifiers are called. But it's done in the
> opposite order: going notifiers are called first, and then
> free_module() is invoked.
> 
> So AFAIU it would be safe to issue the srcu_barrier() from the module
> going notifier.
> 
> Or am I missing something ?

We do seem to be talking past each other.  ;-)

This has n

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

[Mathieu Desnoyers]

- On Apr 7, 2019, at 10:27 PM, paulmck paul...@linux.ibm.com wrote:

> On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
>> > j...@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 paul...@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.

When you say "callbacks arrive after the module text has gone",
I think you assume that free_module() is invoked before the
MODULE_STATE_GOING notifiers are called. But it's done in the
opposite order: going notifiers are called first, and then
free_module() is invoked.

So AFAIU it would be safe to issue the srcu_barrier() from the module
going notifier.

Or am I missing something ?

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mathieu Desnoyers]

- On Apr 7, 2019, at 9:59 AM, paulmck paul...@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 ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Joel Fernandes]

On Fri, Apr 05, 2019 at 04:28:35PM -0700, Paul E. McKenney wrote:
> On Wed, Apr 03, 2019 at 09:20:39AM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> > > - On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:
> > > 
> > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > >> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:
> > > >> 
> > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > >> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com 
> > > >> >> wrote:
> > > >> >> 
> > > >> >> > Hello!
> > > >> >> > 
> > > >> >> > This series prohibits use of DEFINE_SRCU() and 
> > > >> >> > DEFINE_STATIC_SRCU()
> > > >> >> > by loadable modules.  The reason for this prohibition is the fact
> > > >> >> > that using these two macros within modules requires that the size 
> > > >> >> > of
> > > >> >> > the reserved region be increased, which is not something we want 
> > > >> >> > to
> > > >> >> > be doing all that often.  Instead, loadable modules should define 
> > > >> >> > an
> > > >> >> > srcu_struct and invoke init_srcu_struct() from their module_init 
> > > >> >> > function
> > > >> >> > and cleanup_srcu_struct() from their module_exit function.  Note 
> > > >> >> > that
> > > >> >> > modules using call_srcu() will also need to invoke srcu_barrier() 
> > > >> >> > from
> > > >> >> > their module_exit function.
> > > >> >> 
> > > >> >> This arbitrary API limitation seems weird.
> > > >> >> 
> > > >> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> > > >> >> DEFINE_STATIC_SRCU
> > > >> >> while implementing them with dynamic allocation under the hood ?
> > > >> > 
> > > >> > Although call_srcu() already has initialization hooks, some would
> > > >> > also be required in srcu_read_lock(), and I am concerned about adding
> > > >> > memory allocation at that point, especially given the possibility
> > > >> > of memory-allocation failure.  And the possibility that the first
> > > >> > srcu_read_lock() happens in an interrupt handler or similar.
> > > >> > 
> > > >> > Or am I missing a trick here?
> > > >> 
> > > >> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> > > >> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> > > >> would additionally lookup that section on module load, and deal with
> > > >> those statically defined SRCU entries as if they were dynamically
> > > >> allocated ones. It would of course cleanup those resources on module
> > > >> unload.
> > > >> 
> > > >> Am I missing some subtlety there ?
> > > > 
> > > > If I understand you correctly, that is actually what is already done.  
> > > > The
> > > > size of this dedicated section is currently set by 
> > > > PERCPU_MODULE_RESERVE,
> > > > and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring 
> > > > that
> > > > this to be increased frequently.  That led to a request that something
> > > > be done, in turn leading to this patch series.
> > > 
> > > I think we are not expressing quite the same idea.
> > > 
> > > AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data within 
> > > modules,
> > > which ends up using percpu module reserved memory.
> > > 
> > > My idea is to make DEFINE*_SRCU have a different behavior under #ifdef 
> > > MODULE.
> > > It could emit a _global variable_ (_not_ per-cpu) within a new section. 
> > > That
> > > section would then be used by module init/exit code to figure out what 
> > > "srcu
> > > descriptors" are present in the modules. It would therefore rely on 
> > > dynamic
> > > allocation for those, therefore removing the need to involve the percpu 
> > > module
> > > reserved pool at all.
> > > 
> > > > 
> > > > I don't see a way around this short of changing module loading to do
> > > > alloc_percpu() and then updating the relocation based on this result.
> > > > Which would admittedly be far more convenient.  I was assuming that
> > > > this would be difficult due to varying CPU offsets or the like.
> > > > 
> > > > But if it can be done reasonably, it would be quite a bit nicer than
> > > > forcing dynamic allocation in cases where it is not otherwise needed.
> > > 
> > > Hopefully my explanation above helps clear out what I have in mind.
> > > 
> > > You can find similar tricks performed by include/linux/tracepoint.h:
> > > 
> > > #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> > > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > {
> > > return offset_to_ptr(p);
> > > }
> > > 
> > > #define __TRACEPOINT_ENTRY(name)\
> > > asm("   .section \"__tracepoints_ptrs\", \"a\"  \n" \
> > > "   .balign 4   \n" \
> > > "   .long   __tracepoint_" #name " - .  \n" \
> > > "   .previous  

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

[Paul E. McKenney]

On Sat, Apr 06, 2019 at 01:33:27PM +, Joel Fernandes wrote:
> On Fri, Apr 05, 2019 at 04:28:35PM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 03, 2019 at 09:20:39AM -0700, Paul E. McKenney wrote:
> > > On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> > > > - On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:
> > > > 
> > > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > > >> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com 
> > > > >> wrote:
> > > > >> 
> > > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > > >> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com 
> > > > >> >> wrote:
> > > > >> >> 
> > > > >> >> > Hello!
> > > > >> >> > 
> > > > >> >> > This series prohibits use of DEFINE_SRCU() and 
> > > > >> >> > DEFINE_STATIC_SRCU()
> > > > >> >> > by loadable modules.  The reason for this prohibition is the 
> > > > >> >> > fact
> > > > >> >> > that using these two macros within modules requires that the 
> > > > >> >> > size of
> > > > >> >> > the reserved region be increased, which is not something we 
> > > > >> >> > want to
> > > > >> >> > be doing all that often.  Instead, loadable modules should 
> > > > >> >> > define an
> > > > >> >> > srcu_struct and invoke init_srcu_struct() from their 
> > > > >> >> > module_init function
> > > > >> >> > and cleanup_srcu_struct() from their module_exit function.  
> > > > >> >> > Note that
> > > > >> >> > modules using call_srcu() will also need to invoke 
> > > > >> >> > srcu_barrier() from
> > > > >> >> > their module_exit function.
> > > > >> >> 
> > > > >> >> This arbitrary API limitation seems weird.
> > > > >> >> 
> > > > >> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> > > > >> >> DEFINE_STATIC_SRCU
> > > > >> >> while implementing them with dynamic allocation under the hood ?
> > > > >> > 
> > > > >> > Although call_srcu() already has initialization hooks, some would
> > > > >> > also be required in srcu_read_lock(), and I am concerned about 
> > > > >> > adding
> > > > >> > memory allocation at that point, especially given the possibility
> > > > >> > of memory-allocation failure.  And the possibility that the first
> > > > >> > srcu_read_lock() happens in an interrupt handler or similar.
> > > > >> > 
> > > > >> > Or am I missing a trick here?
> > > > >> 
> > > > >> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> > > > >> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> > > > >> would additionally lookup that section on module load, and deal with
> > > > >> those statically defined SRCU entries as if they were dynamically
> > > > >> allocated ones. It would of course cleanup those resources on module
> > > > >> unload.
> > > > >> 
> > > > >> Am I missing some subtlety there ?
> > > > > 
> > > > > If I understand you correctly, that is actually what is already done. 
> > > > >  The
> > > > > size of this dedicated section is currently set by 
> > > > > PERCPU_MODULE_RESERVE,
> > > > > and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring 
> > > > > that
> > > > > this to be increased frequently.  That led to a request that something
> > > > > be done, in turn leading to this patch series.
> > > > 
> > > > I think we are not expressing quite the same idea.
> > > > 
> > > > AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data 
> > > > within modules,
> > > > which ends up using percpu module reserved memory.
> > > > 
> > > > My idea is to make DEFINE*_SRCU have a different behavior under #ifdef 
> > > > MODULE.
> > > > It could emit a _global variable_ (_not_ per-cpu) within a new section. 
> > > > That
> > > > section would then be used by module init/exit code to figure out what 
> > > > "srcu
> > > > descriptors" are present in the modules. It would therefore rely on 
> > > > dynamic
> > > > allocation for those, therefore removing the need to involve the percpu 
> > > > module
> > > > reserved pool at all.
> > > > 
> > > > > 
> > > > > I don't see a way around this short of changing module loading to do
> > > > > alloc_percpu() and then updating the relocation based on this result.
> > > > > Which would admittedly be far more convenient.  I was assuming that
> > > > > this would be difficult due to varying CPU offsets or the like.
> > > > > 
> > > > > But if it can be done reasonably, it would be quite a bit nicer than
> > > > > forcing dynamic allocation in cases where it is not otherwise needed.
> > > > 
> > > > Hopefully my explanation above helps clear out what I have in mind.
> > > > 
> > > > You can find similar tricks performed by include/linux/tracepoint.h:
> > > > 
> > > > #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> > > > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t 
> > > > *p)
> > > > {
> > > > return offset_to_ptr(p);
> > > > }
> > > > 
> > > > #define __TRACEPOINT_ENTRY(name) 

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

[Paul E. McKenney]

On Sun, Apr 07, 2019 at 08:36:46PM -0400, Joel Fernandes wrote:
> On Sun, Apr 07, 2019 at 10:05:14AM -0700, Paul E. McKenney wrote:
> > On Sun, Apr 07, 2019 at 03:46:13PM +, Joel Fernandes wrote:
> > > On Sun, Apr 07, 2019 at 06:59:37AM -0700, Paul E. McKenney 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.
> > > 
> > > Actually the vmlinux.lds.h file is unused for module building. For ex, if 
> > > you
> > > delete include/asm-generic/vmlinux.lds.h , then you can still build
> > > rcutorture.ko. Did I miss something obvious? In that case the 
> > > vmlinux.lds.h
> > > are not needed since the __section annotations automatically place the 
> > > srcu
> > > structs in a separate section.
> > 
> > Hard to argue given that I just deleted include/asm-generic/vmlinux.lds.h,
> > touched kernel/rcu/rcutorture.c, and rebuilt the corresponding .ko
> > without errors.  ;-)
> > 
> > Hmmm...  Is there some way to place a section into a read-only page,
> > for example, tagged onto the text section for that module?  That would
> > get rid of a class of bugs, if nothing else.
> 
> Strictly speaking, the array of pointers in the new srcu section are fixed up
> at runtime because the srcu_struct(s) they point to can be loaded at a
> dynamic location in memory. The srcu_struct(s) are themselves in the .bss
> section of the module and their locations depend on where the .bss section of
> the module is loaded in memory at load time.
> 
> I agree that after such relocation fixups are done, there is no reason to keep
> the array-of-pointers section readable but unfortunately I couldn't figure a
> way out to make them read-only post the relocations.
> 
> I copied Jessica Yu who maintains module loading for any input. Jessica, as a
> summary, we are trying to create a custom ELF section of srcu_struct pointers
> in kernels modules, and then make the module loader do SRCU initialization
> from structs pointed to by this section.  The srcu_struct themselves are 
> defined
> on the .bss section. Is there any way we can make this pointer array section
> read-only *after* the relocation fixups of the array are completed?
> 
> > > Let me know if you would like me to send a patch separately, or if the
> > > appended patch for the same in my previous email suffices.
> > 
> > Please do resend as a formal patch with the above in the commit log.
> > I doubt that I am the only one needing a bit of module-build education!
> > And thank you for providing that education, by the way!
> 
> Sounds great, I will go ahead and send out a patch in the morning for this
> part.

Sounds good on both counts!

Thanx, Paul

> > > > And may I have your Tested-by?
> > > 
> > > Absolutely, please do and thanks!
> > 
> > Done, an

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

[Joel Fernandes]

On Sun, Apr 07, 2019 at 10:05:14AM -0700, Paul E. McKenney wrote:
> On Sun, Apr 07, 2019 at 03:46:13PM +, Joel Fernandes wrote:
> > On Sun, Apr 07, 2019 at 06:59:37AM -0700, Paul E. McKenney 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.
> > 
> > Actually the vmlinux.lds.h file is unused for module building. For ex, if 
> > you
> > delete include/asm-generic/vmlinux.lds.h , then you can still build
> > rcutorture.ko. Did I miss something obvious? In that case the vmlinux.lds.h
> > are not needed since the __section annotations automatically place the srcu
> > structs in a separate section.
> 
> Hard to argue given that I just deleted include/asm-generic/vmlinux.lds.h,
> touched kernel/rcu/rcutorture.c, and rebuilt the corresponding .ko
> without errors.  ;-)
> 
> Hmmm...  Is there some way to place a section into a read-only page,
> for example, tagged onto the text section for that module?  That would
> get rid of a class of bugs, if nothing else.

Strictly speaking, the array of pointers in the new srcu section are fixed up
at runtime because the srcu_struct(s) they point to can be loaded at a
dynamic location in memory. The srcu_struct(s) are themselves in the .bss
section of the module and their locations depend on where the .bss section of
the module is loaded in memory at load time.

I agree that after such relocation fixups are done, there is no reason to keep
the array-of-pointers section readable but unfortunately I couldn't figure a
way out to make them read-only post the relocations.

I copied Jessica Yu who maintains module loading for any input. Jessica, as a
summary, we are trying to create a custom ELF section of srcu_struct pointers
in kernels modules, and then make the module loader do SRCU initialization
from structs pointed to by this section.  The srcu_struct themselves are defined
on the .bss section. Is there any way we can make this pointer array section
read-only *after* the relocation fixups of the array are completed?

> > Let me know if you would like me to send a patch separately, or if the
> > appended patch for the same in my previous email suffices.
> 
> Please do resend as a formal patch with the above in the commit log.
> I doubt that I am the only one needing a bit of module-build education!
> And thank you for providing that education, by the way!

Sounds great, I will go ahead and send out a patch in the morning for this
part.

> > > And may I have your Tested-by?
> > 
> > Absolutely, please do and thanks!
> 
> Done, and thank you for giving it a go!

You're very welcome. thanks,

 - Joel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Joel Fernandes]

On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> - On Apr 7, 2019, at 9:59 AM, paulmck paul...@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.

thanks.


> Thanks,
> 
> Mathieu
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul E. McKenney]

On Wed, Apr 03, 2019 at 09:20:39AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> > - On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:
> > 
> > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > >> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:
> > >> 
> > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > >> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com 
> > >> >> wrote:
> > >> >> 
> > >> >> > Hello!
> > >> >> > 
> > >> >> > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> > >> >> > by loadable modules.  The reason for this prohibition is the fact
> > >> >> > that using these two macros within modules requires that the size of
> > >> >> > the reserved region be increased, which is not something we want to
> > >> >> > be doing all that often.  Instead, loadable modules should define an
> > >> >> > srcu_struct and invoke init_srcu_struct() from their module_init 
> > >> >> > function
> > >> >> > and cleanup_srcu_struct() from their module_exit function.  Note 
> > >> >> > that
> > >> >> > modules using call_srcu() will also need to invoke srcu_barrier() 
> > >> >> > from
> > >> >> > their module_exit function.
> > >> >> 
> > >> >> This arbitrary API limitation seems weird.
> > >> >> 
> > >> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> > >> >> DEFINE_STATIC_SRCU
> > >> >> while implementing them with dynamic allocation under the hood ?
> > >> > 
> > >> > Although call_srcu() already has initialization hooks, some would
> > >> > also be required in srcu_read_lock(), and I am concerned about adding
> > >> > memory allocation at that point, especially given the possibility
> > >> > of memory-allocation failure.  And the possibility that the first
> > >> > srcu_read_lock() happens in an interrupt handler or similar.
> > >> > 
> > >> > Or am I missing a trick here?
> > >> 
> > >> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> > >> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> > >> would additionally lookup that section on module load, and deal with
> > >> those statically defined SRCU entries as if they were dynamically
> > >> allocated ones. It would of course cleanup those resources on module
> > >> unload.
> > >> 
> > >> Am I missing some subtlety there ?
> > > 
> > > If I understand you correctly, that is actually what is already done.  The
> > > size of this dedicated section is currently set by PERCPU_MODULE_RESERVE,
> > > and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring that
> > > this to be increased frequently.  That led to a request that something
> > > be done, in turn leading to this patch series.
> > 
> > I think we are not expressing quite the same idea.
> > 
> > AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data within 
> > modules,
> > which ends up using percpu module reserved memory.
> > 
> > My idea is to make DEFINE*_SRCU have a different behavior under #ifdef 
> > MODULE.
> > It could emit a _global variable_ (_not_ per-cpu) within a new section. That
> > section would then be used by module init/exit code to figure out what "srcu
> > descriptors" are present in the modules. It would therefore rely on dynamic
> > allocation for those, therefore removing the need to involve the percpu 
> > module
> > reserved pool at all.
> > 
> > > 
> > > I don't see a way around this short of changing module loading to do
> > > alloc_percpu() and then updating the relocation based on this result.
> > > Which would admittedly be far more convenient.  I was assuming that
> > > this would be difficult due to varying CPU offsets or the like.
> > > 
> > > But if it can be done reasonably, it would be quite a bit nicer than
> > > forcing dynamic allocation in cases where it is not otherwise needed.
> > 
> > Hopefully my explanation above helps clear out what I have in mind.
> > 
> > You can find similar tricks performed by include/linux/tracepoint.h:
> > 
> > #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > {
> > return offset_to_ptr(p);
> > }
> > 
> > #define __TRACEPOINT_ENTRY(name)\
> > asm("   .section \"__tracepoints_ptrs\", \"a\"  \n" \
> > "   .balign 4   \n" \
> > "   .long   __tracepoint_" #name " - .  \n" \
> > "   .previous   \n")
> > #else
> > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > {
> > return *p;
> > }
> > 
> > #define __TRACEPOINT_ENTRY(name) \
> > static tracepoint_ptr_t __tracepoint_ptr_##name __used   \
> > __attribute__((section("_

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

[Joel Fernandes]

On Fri, Apr 05, 2019 at 04:28:35PM -0700, Paul E. McKenney wrote:
> On Wed, Apr 03, 2019 at 09:20:39AM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> > > - On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:
> > > 
> > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > >> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:
> > > >> 
> > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > >> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com 
> > > >> >> wrote:
> > > >> >> 
> > > >> >> > Hello!
> > > >> >> > 
> > > >> >> > This series prohibits use of DEFINE_SRCU() and 
> > > >> >> > DEFINE_STATIC_SRCU()
> > > >> >> > by loadable modules.  The reason for this prohibition is the fact
> > > >> >> > that using these two macros within modules requires that the size 
> > > >> >> > of
> > > >> >> > the reserved region be increased, which is not something we want 
> > > >> >> > to
> > > >> >> > be doing all that often.  Instead, loadable modules should define 
> > > >> >> > an
> > > >> >> > srcu_struct and invoke init_srcu_struct() from their module_init 
> > > >> >> > function
> > > >> >> > and cleanup_srcu_struct() from their module_exit function.  Note 
> > > >> >> > that
> > > >> >> > modules using call_srcu() will also need to invoke srcu_barrier() 
> > > >> >> > from
> > > >> >> > their module_exit function.
> > > >> >> 
> > > >> >> This arbitrary API limitation seems weird.
> > > >> >> 
> > > >> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> > > >> >> DEFINE_STATIC_SRCU
> > > >> >> while implementing them with dynamic allocation under the hood ?
> > > >> > 
> > > >> > Although call_srcu() already has initialization hooks, some would
> > > >> > also be required in srcu_read_lock(), and I am concerned about adding
> > > >> > memory allocation at that point, especially given the possibility
> > > >> > of memory-allocation failure.  And the possibility that the first
> > > >> > srcu_read_lock() happens in an interrupt handler or similar.
> > > >> > 
> > > >> > Or am I missing a trick here?
> > > >> 
> > > >> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> > > >> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> > > >> would additionally lookup that section on module load, and deal with
> > > >> those statically defined SRCU entries as if they were dynamically
> > > >> allocated ones. It would of course cleanup those resources on module
> > > >> unload.
> > > >> 
> > > >> Am I missing some subtlety there ?
> > > > 
> > > > If I understand you correctly, that is actually what is already done.  
> > > > The
> > > > size of this dedicated section is currently set by 
> > > > PERCPU_MODULE_RESERVE,
> > > > and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring 
> > > > that
> > > > this to be increased frequently.  That led to a request that something
> > > > be done, in turn leading to this patch series.
> > > 
> > > I think we are not expressing quite the same idea.
> > > 
> > > AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data within 
> > > modules,
> > > which ends up using percpu module reserved memory.
> > > 
> > > My idea is to make DEFINE*_SRCU have a different behavior under #ifdef 
> > > MODULE.
> > > It could emit a _global variable_ (_not_ per-cpu) within a new section. 
> > > That
> > > section would then be used by module init/exit code to figure out what 
> > > "srcu
> > > descriptors" are present in the modules. It would therefore rely on 
> > > dynamic
> > > allocation for those, therefore removing the need to involve the percpu 
> > > module
> > > reserved pool at all.
> > > 
> > > > 
> > > > I don't see a way around this short of changing module loading to do
> > > > alloc_percpu() and then updating the relocation based on this result.
> > > > Which would admittedly be far more convenient.  I was assuming that
> > > > this would be difficult due to varying CPU offsets or the like.
> > > > 
> > > > But if it can be done reasonably, it would be quite a bit nicer than
> > > > forcing dynamic allocation in cases where it is not otherwise needed.
> > > 
> > > Hopefully my explanation above helps clear out what I have in mind.
> > > 
> > > You can find similar tricks performed by include/linux/tracepoint.h:
> > > 
> > > #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> > > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > {
> > > return offset_to_ptr(p);
> > > }
> > > 
> > > #define __TRACEPOINT_ENTRY(name)\
> > > asm("   .section \"__tracepoints_ptrs\", \"a\"  \n" \
> > > "   .balign 4   \n" \
> > > "   .long   __tracepoint_" #name " - .  \n" \
> > > "   .previous  

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

[Paul E. McKenney]

On Sun, Apr 07, 2019 at 09:07:18PM +, 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 
> > j...@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 paul...@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.

Thanx, Paul

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Joel Fernandes]

On Sun, Apr 07, 2019 at 06:59:37AM -0700, Paul E. McKenney 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.
>

Actually the vmlinux.lds.h file is unused for module building. For ex, if you
delete include/asm-generic/vmlinux.lds.h , then you can still build
rcutorture.ko. Did I miss something obvious? In that case the vmlinux.lds.h
are not needed since the __section annotations automatically place the srcu
structs in a separate section.

Let me know if you would like me to send a patch separately, or if the
appended patch for the same in my previous email suffices.

> And may I have your Tested-by?

Absolutely, please do and thanks!

 - Joel


>   Thanx, Paul
> 
> 
> 
> commit a365bb5f6eafb220a1448674054b05c250829313
> Author: Paul E. McKenney 
> Date:   Fri Apr 5 16:15:00 2019 -0700
> 
> srcu: Allocate per-CPU data for DEFINE_SRCU() in modules
> 
> Adding DEFINE_SRCU() or DEFINE_STATIC_SRCU() to a loadable module requires
> that the size of the reserved region be increased, which is not something
> we want to be doing all that often.  One approach would be to require
> that loadable modules define an srcu_struct and invoke init_srcu_struct()
> from their module_init function and cleanup_srcu_struct() from their
> module_exit function.  However, this is more than a bit user unfriendly.
> 
> This commit therefore creates an ___srcu_struct_ptrs linker section,
> and pointers to srcu_struct structures created by DEFINE_SRCU() and
> DEFINE_STATIC_SRCU() within a module are placed into that module's
> ___srcu_struct_ptrs section.  The required init_srcu_struct() and
> cleanup_srcu_struct() functions are then automatically invoked as needed
> when that module is loaded and unloaded, thus allowing modules to continue
> to use DEFINE_SRCU() and DEFINE_STATIC_SRCU() while avoiding the need
> to increase the size of the reserved region.
> 
> Many of the algorithms and some of the code was cheerfully cherry-picked
> from other code making use of linker sections, perhaps most notably from
> tracepoints.  All bugs are nevertheless the sole property of the author.
> 
> Suggested-by: Mathieu Desnoyers 
> [ paulmck: Use __section() and use "default" in srcu_module_notify()'s
>   "switch" statement as suggested by Joel Fernandes. ]
> Signed-off-by: Paul E. McKenney 
> 
> 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);  

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

[Joel Fernandes]

On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> 
> - On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google 
> j...@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 paul...@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.

thanks!

> 
> Thanks,
> 
> Mathieu
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul E. McKenney]

On Sun, Apr 07, 2019 at 03:46:13PM +, Joel Fernandes wrote:
> On Sun, Apr 07, 2019 at 06:59:37AM -0700, Paul E. McKenney 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.
> 
> Actually the vmlinux.lds.h file is unused for module building. For ex, if you
> delete include/asm-generic/vmlinux.lds.h , then you can still build
> rcutorture.ko. Did I miss something obvious? In that case the vmlinux.lds.h
> are not needed since the __section annotations automatically place the srcu
> structs in a separate section.

Hard to argue given that I just deleted include/asm-generic/vmlinux.lds.h,
touched kernel/rcu/rcutorture.c, and rebuilt the corresponding .ko
without errors.  ;-)

Hmmm...  Is there some way to place a section into a read-only page,
for example, tagged onto the text section for that module?  That would
get rid of a class of bugs, if nothing else.

> Let me know if you would like me to send a patch separately, or if the
> appended patch for the same in my previous email suffices.

Please do resend as a formal patch with the above in the commit log.
I doubt that I am the only one needing a bit of module-build education!
And thank you for providing that education, by the way!

> > And may I have your Tested-by?
> 
> Absolutely, please do and thanks!

Done, and thank you for giving it a go!

Thanx, Paul

>  - Joel
> 
> 
> > Thanx, Paul
> > 
> > 
> > 
> > commit a365bb5f6eafb220a1448674054b05c250829313
> > Author: Paul E. McKenney 
> > Date:   Fri Apr 5 16:15:00 2019 -0700
> > 
> > srcu: Allocate per-CPU data for DEFINE_SRCU() in modules
> > 
> > Adding DEFINE_SRCU() or DEFINE_STATIC_SRCU() to a loadable module 
> > requires
> > that the size of the reserved region be increased, which is not 
> > something
> > we want to be doing all that often.  One approach would be to require
> > that loadable modules define an srcu_struct and invoke 
> > init_srcu_struct()
> > from their module_init function and cleanup_srcu_struct() from their
> > module_exit function.  However, this is more than a bit user unfriendly.
> > 
> > This commit therefore creates an ___srcu_struct_ptrs linker section,
> > and pointers to srcu_struct structures created by DEFINE_SRCU() and
> > DEFINE_STATIC_SRCU() within a module are placed into that module's
> > ___srcu_struct_ptrs section.  The required init_srcu_struct() and
> > cleanup_srcu_struct() functions are then automatically invoked as needed
> > when that module is loaded and unloaded, thus allowing modules to 
> > continue
> > to use DEFINE_SRCU() and DEFINE_STATIC_SRCU() while avoiding the need
> > to increase the size of the reserved region.
> > 
> > Many of the algorithms and some of the code was cheerfully cherry-pick

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

[Paul E. McKenney]

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?

Thanx, Paul



commit a365bb5f6eafb220a1448674054b05c250829313
Author: Paul E. McKenney 
Date:   Fri Apr 5 16:15:00 2019 -0700

srcu: Allocate per-CPU data for DEFINE_SRCU() in modules

Adding DEFINE_SRCU() or DEFINE_STATIC_SRCU() to a loadable module requires
that the size of the reserved region be increased, which is not something
we want to be doing all that often.  One approach would be to require
that loadable modules define an srcu_struct and invoke init_srcu_struct()
from their module_init function and cleanup_srcu_struct() from their
module_exit function.  However, this is more than a bit user unfriendly.

This commit therefore creates an ___srcu_struct_ptrs linker section,
and pointers to srcu_struct structures created by DEFINE_SRCU() and
DEFINE_STATIC_SRCU() within a module are placed into that module's
___srcu_struct_ptrs section.  The required init_srcu_struct() and
cleanup_srcu_struct() functions are then automatically invoked as needed
when that module is loaded and unloaded, thus allowing modules to continue
to use DEFINE_SRCU() and DEFINE_STATIC_SRCU() while avoiding the need
to increase the size of the reserved region.

Many of the algorithms and some of the code was cheerfully cherry-picked
from other code making use of linker sections, perhaps most notably from
tracepoints.  All bugs are nevertheless the sole property of the author.

Suggested-by: Mathieu Desnoyers 
[ paulmck: Use __section() and use "default" in srcu_module_notify()'s
  "switch" statement as suggested by Joel Fernandes. ]
Signed-off-by: Paul E. McKenney 

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 = .;\
}   \
\
.rodata1  : AT(ADDR(.rodata1) - LOAD_OFFSET) {  \
diff --git a/include/linux/module.h b/include/linux/module.h
index 5bf5dcd91009..921443a026dd 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -448,6 +449,10 @@ struct module {
unsigned int num_tracepoints;
tracepoint_ptr_t *tracepoints_ptrs;
 #endif
+#ifdef CONFIG_TREE_SRCU
+   unsi

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

[Mathieu Desnoyers]

- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google j...@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 paul...@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 ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul E. McKenney]

On Sat, Apr 06, 2019 at 07:06:13PM -0400, Joel Fernandes wrote:
> On Fri, Apr 05, 2019 at 04:28:35PM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 03, 2019 at 09:20:39AM -0700, Paul E. McKenney wrote:
> > > On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> > > > - On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:
> > > > 
> > > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > > >> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com 
> > > > >> wrote:
> > > > >> 
> > > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > > >> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com 
> > > > >> >> wrote:
> > > > >> >> 
> > > > >> >> > Hello!
> > > > >> >> > 
> > > > >> >> > This series prohibits use of DEFINE_SRCU() and 
> > > > >> >> > DEFINE_STATIC_SRCU()
> > > > >> >> > by loadable modules.  The reason for this prohibition is the 
> > > > >> >> > fact
> > > > >> >> > that using these two macros within modules requires that the 
> > > > >> >> > size of
> > > > >> >> > the reserved region be increased, which is not something we 
> > > > >> >> > want to
> > > > >> >> > be doing all that often.  Instead, loadable modules should 
> > > > >> >> > define an
> > > > >> >> > srcu_struct and invoke init_srcu_struct() from their 
> > > > >> >> > module_init function
> > > > >> >> > and cleanup_srcu_struct() from their module_exit function.  
> > > > >> >> > Note that
> > > > >> >> > modules using call_srcu() will also need to invoke 
> > > > >> >> > srcu_barrier() from
> > > > >> >> > their module_exit function.
> > > > >> >> 
> > > > >> >> This arbitrary API limitation seems weird.
> > > > >> >> 
> > > > >> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> > > > >> >> DEFINE_STATIC_SRCU
> > > > >> >> while implementing them with dynamic allocation under the hood ?
> > > > >> > 
> > > > >> > Although call_srcu() already has initialization hooks, some would
> > > > >> > also be required in srcu_read_lock(), and I am concerned about 
> > > > >> > adding
> > > > >> > memory allocation at that point, especially given the possibility
> > > > >> > of memory-allocation failure.  And the possibility that the first
> > > > >> > srcu_read_lock() happens in an interrupt handler or similar.
> > > > >> > 
> > > > >> > Or am I missing a trick here?
> > > > >> 
> > > > >> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> > > > >> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> > > > >> would additionally lookup that section on module load, and deal with
> > > > >> those statically defined SRCU entries as if they were dynamically
> > > > >> allocated ones. It would of course cleanup those resources on module
> > > > >> unload.
> > > > >> 
> > > > >> Am I missing some subtlety there ?
> > > > > 
> > > > > If I understand you correctly, that is actually what is already done. 
> > > > >  The
> > > > > size of this dedicated section is currently set by 
> > > > > PERCPU_MODULE_RESERVE,
> > > > > and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring 
> > > > > that
> > > > > this to be increased frequently.  That led to a request that something
> > > > > be done, in turn leading to this patch series.
> > > > 
> > > > I think we are not expressing quite the same idea.
> > > > 
> > > > AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data 
> > > > within modules,
> > > > which ends up using percpu module reserved memory.
> > > > 
> > > > My idea is to make DEFINE*_SRCU have a different behavior under #ifdef 
> > > > MODULE.
> > > > It could emit a _global variable_ (_not_ per-cpu) within a new section. 
> > > > That
> > > > section would then be used by module init/exit code to figure out what 
> > > > "srcu
> > > > descriptors" are present in the modules. It would therefore rely on 
> > > > dynamic
> > > > allocation for those, therefore removing the need to involve the percpu 
> > > > module
> > > > reserved pool at all.
> > > > 
> > > > > 
> > > > > I don't see a way around this short of changing module loading to do
> > > > > alloc_percpu() and then updating the relocation based on this result.
> > > > > Which would admittedly be far more convenient.  I was assuming that
> > > > > this would be difficult due to varying CPU offsets or the like.
> > > > > 
> > > > > But if it can be done reasonably, it would be quite a bit nicer than
> > > > > forcing dynamic allocation in cases where it is not otherwise needed.
> > > > 
> > > > Hopefully my explanation above helps clear out what I have in mind.
> > > > 
> > > > You can find similar tricks performed by include/linux/tracepoint.h:
> > > > 
> > > > #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> > > > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t 
> > > > *p)
> > > > {
> > > > return offset_to_ptr(p);
> > > > }
> > > > 
> > > > #define __TRACEPOINT_ENTRY(name) 

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

[Mathieu Desnoyers]

- On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:

> On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
>> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:
>> 
>> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
>> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:
>> >> 
>> >> > Hello!
>> >> > 
>> >> > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
>> >> > by loadable modules.  The reason for this prohibition is the fact
>> >> > that using these two macros within modules requires that the size of
>> >> > the reserved region be increased, which is not something we want to
>> >> > be doing all that often.  Instead, loadable modules should define an
>> >> > srcu_struct and invoke init_srcu_struct() from their module_init 
>> >> > function
>> >> > and cleanup_srcu_struct() from their module_exit function.  Note that
>> >> > modules using call_srcu() will also need to invoke srcu_barrier() from
>> >> > their module_exit function.
>> >> 
>> >> This arbitrary API limitation seems weird.
>> >> 
>> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
>> >> DEFINE_STATIC_SRCU
>> >> while implementing them with dynamic allocation under the hood ?
>> > 
>> > Although call_srcu() already has initialization hooks, some would
>> > also be required in srcu_read_lock(), and I am concerned about adding
>> > memory allocation at that point, especially given the possibility
>> > of memory-allocation failure.  And the possibility that the first
>> > srcu_read_lock() happens in an interrupt handler or similar.
>> > 
>> > Or am I missing a trick here?
>> 
>> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
>> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
>> would additionally lookup that section on module load, and deal with
>> those statically defined SRCU entries as if they were dynamically
>> allocated ones. It would of course cleanup those resources on module
>> unload.
>> 
>> Am I missing some subtlety there ?
> 
> If I understand you correctly, that is actually what is already done.  The
> size of this dedicated section is currently set by PERCPU_MODULE_RESERVE,
> and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring that
> this to be increased frequently.  That led to a request that something
> be done, in turn leading to this patch series.

I think we are not expressing quite the same idea.

AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data within 
modules,
which ends up using percpu module reserved memory.

My idea is to make DEFINE*_SRCU have a different behavior under #ifdef MODULE.
It could emit a _global variable_ (_not_ per-cpu) within a new section. That
section would then be used by module init/exit code to figure out what "srcu
descriptors" are present in the modules. It would therefore rely on dynamic
allocation for those, therefore removing the need to involve the percpu module
reserved pool at all.

> 
> I don't see a way around this short of changing module loading to do
> alloc_percpu() and then updating the relocation based on this result.
> Which would admittedly be far more convenient.  I was assuming that
> this would be difficult due to varying CPU offsets or the like.
> 
> But if it can be done reasonably, it would be quite a bit nicer than
> forcing dynamic allocation in cases where it is not otherwise needed.

Hopefully my explanation above helps clear out what I have in mind.

You can find similar tricks performed by include/linux/tracepoint.h:

#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
{
return offset_to_ptr(p);
}

#define __TRACEPOINT_ENTRY(name)\
asm("   .section \"__tracepoints_ptrs\", \"a\"  \n" \
"   .balign 4   \n" \
"   .long   __tracepoint_" #name " - .  \n" \
"   .previous   \n")
#else
static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
{
return *p;
}

#define __TRACEPOINT_ENTRY(name) \
static tracepoint_ptr_t __tracepoint_ptr_##name __used   \
__attribute__((section("__tracepoints_ptrs"))) = \
&__tracepoint_##name
#endif

[...]

#define DEFINE_TRACE_FN(name, reg, unreg)\
static const char __tpstrtab_##name[]\
__attribute__((section("__tracepoints_strings"))) = #name;   \
struct tracepoint __tracepoint_##name\
__attribute__((section("__tracepoints"), used)) =\
{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\
__TRACEP

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

[Paul E. McKenney]

On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> - On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:
> 
> > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> >> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:
> >> 
> >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> >> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:
> >> >> 
> >> >> > Hello!
> >> >> > 
> >> >> > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> >> >> > by loadable modules.  The reason for this prohibition is the fact
> >> >> > that using these two macros within modules requires that the size of
> >> >> > the reserved region be increased, which is not something we want to
> >> >> > be doing all that often.  Instead, loadable modules should define an
> >> >> > srcu_struct and invoke init_srcu_struct() from their module_init 
> >> >> > function
> >> >> > and cleanup_srcu_struct() from their module_exit function.  Note that
> >> >> > modules using call_srcu() will also need to invoke srcu_barrier() from
> >> >> > their module_exit function.
> >> >> 
> >> >> This arbitrary API limitation seems weird.
> >> >> 
> >> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> >> >> DEFINE_STATIC_SRCU
> >> >> while implementing them with dynamic allocation under the hood ?
> >> > 
> >> > Although call_srcu() already has initialization hooks, some would
> >> > also be required in srcu_read_lock(), and I am concerned about adding
> >> > memory allocation at that point, especially given the possibility
> >> > of memory-allocation failure.  And the possibility that the first
> >> > srcu_read_lock() happens in an interrupt handler or similar.
> >> > 
> >> > Or am I missing a trick here?
> >> 
> >> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> >> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> >> would additionally lookup that section on module load, and deal with
> >> those statically defined SRCU entries as if they were dynamically
> >> allocated ones. It would of course cleanup those resources on module
> >> unload.
> >> 
> >> Am I missing some subtlety there ?
> > 
> > If I understand you correctly, that is actually what is already done.  The
> > size of this dedicated section is currently set by PERCPU_MODULE_RESERVE,
> > and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring that
> > this to be increased frequently.  That led to a request that something
> > be done, in turn leading to this patch series.
> 
> I think we are not expressing quite the same idea.
> 
> AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data within 
> modules,
> which ends up using percpu module reserved memory.
> 
> My idea is to make DEFINE*_SRCU have a different behavior under #ifdef MODULE.
> It could emit a _global variable_ (_not_ per-cpu) within a new section. That
> section would then be used by module init/exit code to figure out what "srcu
> descriptors" are present in the modules. It would therefore rely on dynamic
> allocation for those, therefore removing the need to involve the percpu module
> reserved pool at all.
> 
> > 
> > I don't see a way around this short of changing module loading to do
> > alloc_percpu() and then updating the relocation based on this result.
> > Which would admittedly be far more convenient.  I was assuming that
> > this would be difficult due to varying CPU offsets or the like.
> > 
> > But if it can be done reasonably, it would be quite a bit nicer than
> > forcing dynamic allocation in cases where it is not otherwise needed.
> 
> Hopefully my explanation above helps clear out what I have in mind.
> 
> You can find similar tricks performed by include/linux/tracepoint.h:
> 
> #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> {
> return offset_to_ptr(p);
> }
> 
> #define __TRACEPOINT_ENTRY(name)\
> asm("   .section \"__tracepoints_ptrs\", \"a\"  \n" \
> "   .balign 4   \n" \
> "   .long   __tracepoint_" #name " - .  \n" \
> "   .previous   \n")
> #else
> static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> {
> return *p;
> }
> 
> #define __TRACEPOINT_ENTRY(name) \
> static tracepoint_ptr_t __tracepoint_ptr_##name __used   \
> __attribute__((section("__tracepoints_ptrs"))) = \
> &__tracepoint_##name
> #endif
> 
> [...]
> 
> #define DEFINE_TRACE_FN(name, reg, unreg)\
> static const char __tpstrtab_##name[]\
> __attribute__((section("__tracepoints_s

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

[Paul E. McKenney]

On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:
> 
> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:
> >> 
> >> > Hello!
> >> > 
> >> > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> >> > by loadable modules.  The reason for this prohibition is the fact
> >> > that using these two macros within modules requires that the size of
> >> > the reserved region be increased, which is not something we want to
> >> > be doing all that often.  Instead, loadable modules should define an
> >> > srcu_struct and invoke init_srcu_struct() from their module_init function
> >> > and cleanup_srcu_struct() from their module_exit function.  Note that
> >> > modules using call_srcu() will also need to invoke srcu_barrier() from
> >> > their module_exit function.
> >> 
> >> This arbitrary API limitation seems weird.
> >> 
> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> >> DEFINE_STATIC_SRCU
> >> while implementing them with dynamic allocation under the hood ?
> > 
> > Although call_srcu() already has initialization hooks, some would
> > also be required in srcu_read_lock(), and I am concerned about adding
> > memory allocation at that point, especially given the possibility
> > of memory-allocation failure.  And the possibility that the first
> > srcu_read_lock() happens in an interrupt handler or similar.
> > 
> > Or am I missing a trick here?
> 
> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> would additionally lookup that section on module load, and deal with
> those statically defined SRCU entries as if they were dynamically
> allocated ones. It would of course cleanup those resources on module
> unload.
> 
> Am I missing some subtlety there ?

If I understand you correctly, that is actually what is already done.  The
size of this dedicated section is currently set by PERCPU_MODULE_RESERVE,
and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring that
this to be increased frequently.  That led to a request that something
be done, in turn leading to this patch series.

I don't see a way around this short of changing module loading to do
alloc_percpu() and then updating the relocation based on this result.
Which would admittedly be far more convenient.  I was assuming that
this would be difficult due to varying CPU offsets or the like.

But if it can be done reasonably, it would be quite a bit nicer than
forcing dynamic allocation in cases where it is not otherwise needed.

Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> > Thanx, Paul
> > 
> >> Thanks,
> >> 
> >> Mathieu
> >> 
> >> 
> >> > 
> >> > This series consist of the following:
> >> > 
> >> > 1.   Dynamically allocate dax_srcu.
> >> > 
> >> > 2.   Dynamically allocate drm_unplug_srcu.
> >> > 
> >> > 3.   Dynamically allocate kfd_processes_srcu.
> >> > 
> >> > These build and have been subjected to 0day testing, but might also need
> >> > testing by someone having the requisite hardware.
> >> > 
> >> >  Thanx, Paul
> >> > 
> >> > 
> >> > 
> >> > drivers/dax/super.c|   10 +-
> >> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c |5 +++
> >> > drivers/gpu/drm/amd/amdkfd/kfd_process.c   |2 -
> >> > drivers/gpu/drm/drm_drv.c  |8 
> >> > include/linux/srcutree.h   |   19 +--
> >> > kernel/rcu/rcuperf.c   |   40 
> >> > +++-
> >> > kernel/rcu/rcutorture.c|   48 
> >> > +
> >> >  7 files changed, 105 insertions(+), 27 deletions(-)
> >> 
> >> --
> >> Mathieu Desnoyers
> >> EfficiOS Inc.
> >> http://www.efficios.com
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Joel Fernandes]

On Wed, Apr 03, 2019 at 09:20:39AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> > - On Apr 3, 2019, at 9:32 AM, paulmck paul...@linux.ibm.com wrote:
> > 
> > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > >> - On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:
> > >> 
> > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > >> >> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com 
> > >> >> wrote:
> > >> >> 
> > >> >> > Hello!
> > >> >> > 
> > >> >> > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> > >> >> > by loadable modules.  The reason for this prohibition is the fact
> > >> >> > that using these two macros within modules requires that the size of
> > >> >> > the reserved region be increased, which is not something we want to
> > >> >> > be doing all that often.  Instead, loadable modules should define an
> > >> >> > srcu_struct and invoke init_srcu_struct() from their module_init 
> > >> >> > function
> > >> >> > and cleanup_srcu_struct() from their module_exit function.  Note 
> > >> >> > that
> > >> >> > modules using call_srcu() will also need to invoke srcu_barrier() 
> > >> >> > from
> > >> >> > their module_exit function.
> > >> >> 
> > >> >> This arbitrary API limitation seems weird.
> > >> >> 
> > >> >> Isn't there a way to allow modules to use DEFINE_SRCU and 
> > >> >> DEFINE_STATIC_SRCU
> > >> >> while implementing them with dynamic allocation under the hood ?
> > >> > 
> > >> > Although call_srcu() already has initialization hooks, some would
> > >> > also be required in srcu_read_lock(), and I am concerned about adding
> > >> > memory allocation at that point, especially given the possibility
> > >> > of memory-allocation failure.  And the possibility that the first
> > >> > srcu_read_lock() happens in an interrupt handler or similar.
> > >> > 
> > >> > Or am I missing a trick here?
> > >> 
> > >> I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
> > >> DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
> > >> would additionally lookup that section on module load, and deal with
> > >> those statically defined SRCU entries as if they were dynamically
> > >> allocated ones. It would of course cleanup those resources on module
> > >> unload.
> > >> 
> > >> Am I missing some subtlety there ?
> > > 
> > > If I understand you correctly, that is actually what is already done.  The
> > > size of this dedicated section is currently set by PERCPU_MODULE_RESERVE,
> > > and the additions of DEFINE{_STATIC}_SRCU() in modules was requiring that
> > > this to be increased frequently.  That led to a request that something
> > > be done, in turn leading to this patch series.
> > 
> > I think we are not expressing quite the same idea.
> > 
> > AFAIU, yours is to have DEFINE*_SRCU directly define per-cpu data within 
> > modules,
> > which ends up using percpu module reserved memory.
> > 
> > My idea is to make DEFINE*_SRCU have a different behavior under #ifdef 
> > MODULE.
> > It could emit a _global variable_ (_not_ per-cpu) within a new section. That
> > section would then be used by module init/exit code to figure out what "srcu
> > descriptors" are present in the modules. It would therefore rely on dynamic
> > allocation for those, therefore removing the need to involve the percpu 
> > module
> > reserved pool at all.
> > 
> > > 
> > > I don't see a way around this short of changing module loading to do
> > > alloc_percpu() and then updating the relocation based on this result.
> > > Which would admittedly be far more convenient.  I was assuming that
> > > this would be difficult due to varying CPU offsets or the like.
> > > 
> > > But if it can be done reasonably, it would be quite a bit nicer than
> > > forcing dynamic allocation in cases where it is not otherwise needed.
> > 
> > Hopefully my explanation above helps clear out what I have in mind.
> > 
> > You can find similar tricks performed by include/linux/tracepoint.h:
> > 
> > #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > {
> > return offset_to_ptr(p);
> > }
> > 
> > #define __TRACEPOINT_ENTRY(name)\
> > asm("   .section \"__tracepoints_ptrs\", \"a\"  \n" \
> > "   .balign 4   \n" \
> > "   .long   __tracepoint_" #name " - .  \n" \
> > "   .previous   \n")
> > #else
> > static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > {
> > return *p;
> > }
> > 
> > #define __TRACEPOINT_ENTRY(name) \
> > static tracepoint_ptr_t __tracepoint_ptr_##name __used   \
> > __attribute__((section("_

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

[Paul E. McKenney]

On Tue, Apr 02, 2019 at 02:40:54PM -0400, Joel Fernandes wrote:
> On Tue, Apr 02, 2019 at 08:23:34AM -0700, Paul E. McKenney wrote:
> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:
> > > 
> > > > Hello!
> > > > 
> > > > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> > > > by loadable modules.  The reason for this prohibition is the fact
> > > > that using these two macros within modules requires that the size of
> > > > the reserved region be increased, which is not something we want to
> > > > be doing all that often.  Instead, loadable modules should define an
> > > > srcu_struct and invoke init_srcu_struct() from their module_init 
> > > > function
> > > > and cleanup_srcu_struct() from their module_exit function.  Note that
> > > > modules using call_srcu() will also need to invoke srcu_barrier() from
> > > > their module_exit function.
> > > 
> > > This arbitrary API limitation seems weird.
> > > 
> > > Isn't there a way to allow modules to use DEFINE_SRCU and 
> > > DEFINE_STATIC_SRCU
> > > while implementing them with dynamic allocation under the hood ?
> > 
> > Although call_srcu() already has initialization hooks, some would
> > also be required in srcu_read_lock(), and I am concerned about adding
> > memory allocation at that point, especially given the possibility
> > of memory-allocation failure.  And the possibility that the first
> > srcu_read_lock() happens in an interrupt handler or similar.
> > 
> > Or am I missing a trick here?
> 
> Hi Paul,
> 
> Which 'reserved region' are you referring to? Isn't this region also
> used for non-module cases in which case the same problem applies to
> non-modules?

The percpu/module reservation discussed in this thread:

https://lore.kernel.org/lkml/c72402f2-967e-cd56-99d8-9139c9e7f...@google.com/T/#mbcacf60ddc0b3fd6e831a3ea71efc90da359a3bf

For non-modules, global per-CPU variables are statically allocated.
For modules, they must be dynamically allocated at modprobe time, and
their size is set by PERCPU_MODULE_RESERVE.

Thanx, Paul

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mathieu Desnoyers]

- On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:

> Hello!
> 
> This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> by loadable modules.  The reason for this prohibition is the fact
> that using these two macros within modules requires that the size of
> the reserved region be increased, which is not something we want to
> be doing all that often.  Instead, loadable modules should define an
> srcu_struct and invoke init_srcu_struct() from their module_init function
> and cleanup_srcu_struct() from their module_exit function.  Note that
> modules using call_srcu() will also need to invoke srcu_barrier() from
> their module_exit function.

This arbitrary API limitation seems weird.

Isn't there a way to allow modules to use DEFINE_SRCU and DEFINE_STATIC_SRCU
while implementing them with dynamic allocation under the hood ?

Thanks,

Mathieu


> 
> This series consist of the following:
> 
> 1.Dynamically allocate dax_srcu.
> 
> 2.Dynamically allocate drm_unplug_srcu.
> 
> 3.Dynamically allocate kfd_processes_srcu.
> 
> These build and have been subjected to 0day testing, but might also need
> testing by someone having the requisite hardware.
> 
>   Thanx, Paul
> 
> 
> 
> drivers/dax/super.c|   10 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c |5 +++
> drivers/gpu/drm/amd/amdkfd/kfd_process.c   |2 -
> drivers/gpu/drm/drm_drv.c  |8 
> include/linux/srcutree.h   |   19 +--
> kernel/rcu/rcuperf.c   |   40 +++-
> kernel/rcu/rcutorture.c|   48 
> +
>  7 files changed, 105 insertions(+), 27 deletions(-)

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul E. McKenney]

On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:
> 
> > Hello!
> > 
> > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> > by loadable modules.  The reason for this prohibition is the fact
> > that using these two macros within modules requires that the size of
> > the reserved region be increased, which is not something we want to
> > be doing all that often.  Instead, loadable modules should define an
> > srcu_struct and invoke init_srcu_struct() from their module_init function
> > and cleanup_srcu_struct() from their module_exit function.  Note that
> > modules using call_srcu() will also need to invoke srcu_barrier() from
> > their module_exit function.
> 
> This arbitrary API limitation seems weird.
> 
> Isn't there a way to allow modules to use DEFINE_SRCU and DEFINE_STATIC_SRCU
> while implementing them with dynamic allocation under the hood ?

Although call_srcu() already has initialization hooks, some would
also be required in srcu_read_lock(), and I am concerned about adding
memory allocation at that point, especially given the possibility
of memory-allocation failure.  And the possibility that the first
srcu_read_lock() happens in an interrupt handler or similar.

Or am I missing a trick here?

Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> > This series consist of the following:
> > 
> > 1.  Dynamically allocate dax_srcu.
> > 
> > 2.  Dynamically allocate drm_unplug_srcu.
> > 
> > 3.  Dynamically allocate kfd_processes_srcu.
> > 
> > These build and have been subjected to 0day testing, but might also need
> > testing by someone having the requisite hardware.
> > 
> > Thanx, Paul
> > 
> > 
> > 
> > drivers/dax/super.c|   10 +-
> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c |5 +++
> > drivers/gpu/drm/amd/amdkfd/kfd_process.c   |2 -
> > drivers/gpu/drm/drm_drv.c  |8 
> > include/linux/srcutree.h   |   19 +--
> > kernel/rcu/rcuperf.c   |   40 +++-
> > kernel/rcu/rcutorture.c|   48 
> > +
> >  7 files changed, 105 insertions(+), 27 deletions(-)
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Joel Fernandes]

On Tue, Apr 02, 2019 at 08:23:34AM -0700, Paul E. McKenney wrote:
> On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:
> > 
> > > Hello!
> > > 
> > > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
> > > by loadable modules.  The reason for this prohibition is the fact
> > > that using these two macros within modules requires that the size of
> > > the reserved region be increased, which is not something we want to
> > > be doing all that often.  Instead, loadable modules should define an
> > > srcu_struct and invoke init_srcu_struct() from their module_init function
> > > and cleanup_srcu_struct() from their module_exit function.  Note that
> > > modules using call_srcu() will also need to invoke srcu_barrier() from
> > > their module_exit function.
> > 
> > This arbitrary API limitation seems weird.
> > 
> > Isn't there a way to allow modules to use DEFINE_SRCU and DEFINE_STATIC_SRCU
> > while implementing them with dynamic allocation under the hood ?
> 
> Although call_srcu() already has initialization hooks, some would
> also be required in srcu_read_lock(), and I am concerned about adding
> memory allocation at that point, especially given the possibility
> of memory-allocation failure.  And the possibility that the first
> srcu_read_lock() happens in an interrupt handler or similar.
> 
> Or am I missing a trick here?

Hi Paul,

Which 'reserved region' are you referring to? Isn't this region also
used for non-module cases in which case the same problem applies to
non-modules?

thanks!

 - Joel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mathieu Desnoyers]

- On Apr 2, 2019, at 11:23 AM, paulmck paul...@linux.ibm.com wrote:

> On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
>> - On Apr 2, 2019, at 10:28 AM, paulmck paul...@linux.ibm.com wrote:
>> 
>> > Hello!
>> > 
>> > This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
>> > by loadable modules.  The reason for this prohibition is the fact
>> > that using these two macros within modules requires that the size of
>> > the reserved region be increased, which is not something we want to
>> > be doing all that often.  Instead, loadable modules should define an
>> > srcu_struct and invoke init_srcu_struct() from their module_init function
>> > and cleanup_srcu_struct() from their module_exit function.  Note that
>> > modules using call_srcu() will also need to invoke srcu_barrier() from
>> > their module_exit function.
>> 
>> This arbitrary API limitation seems weird.
>> 
>> Isn't there a way to allow modules to use DEFINE_SRCU and DEFINE_STATIC_SRCU
>> while implementing them with dynamic allocation under the hood ?
> 
> Although call_srcu() already has initialization hooks, some would
> also be required in srcu_read_lock(), and I am concerned about adding
> memory allocation at that point, especially given the possibility
> of memory-allocation failure.  And the possibility that the first
> srcu_read_lock() happens in an interrupt handler or similar.
> 
> Or am I missing a trick here?

I was more thinking that under #ifdef MODULE, both DEFINE_SRCU and
DEFINE_STATIC_SRCU could append data in a dedicated section. module.c
would additionally lookup that section on module load, and deal with
those statically defined SRCU entries as if they were dynamically
allocated ones. It would of course cleanup those resources on module
unload.

Am I missing some subtlety there ?

Thanks,

Mathieu


> 
>   Thanx, Paul
> 
>> Thanks,
>> 
>> Mathieu
>> 
>> 
>> > 
>> > This series consist of the following:
>> > 
>> > 1. Dynamically allocate dax_srcu.
>> > 
>> > 2. Dynamically allocate drm_unplug_srcu.
>> > 
>> > 3. Dynamically allocate kfd_processes_srcu.
>> > 
>> > These build and have been subjected to 0day testing, but might also need
>> > testing by someone having the requisite hardware.
>> > 
>> >Thanx, Paul
>> > 
>> > 
>> > 
>> > drivers/dax/super.c|   10 +-
>> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c |5 +++
>> > drivers/gpu/drm/amd/amdkfd/kfd_process.c   |2 -
>> > drivers/gpu/drm/drm_drv.c  |8 
>> > include/linux/srcutree.h   |   19 +--
>> > kernel/rcu/rcuperf.c   |   40 +++-
>> > kernel/rcu/rcutorture.c|   48 
>> > +
>> >  7 files changed, 105 insertions(+), 27 deletions(-)
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul E. McKenney]

Hello!

This series prohibits use of DEFINE_SRCU() and DEFINE_STATIC_SRCU()
by loadable modules.  The reason for this prohibition is the fact
that using these two macros within modules requires that the size of
the reserved region be increased, which is not something we want to
be doing all that often.  Instead, loadable modules should define an
srcu_struct and invoke init_srcu_struct() from their module_init function
and cleanup_srcu_struct() from their module_exit function.  Note that
modules using call_srcu() will also need to invoke srcu_barrier() from
their module_exit function.

This series consist of the following:

1.  Dynamically allocate dax_srcu.

2.  Dynamically allocate drm_unplug_srcu.

3.  Dynamically allocate kfd_processes_srcu.

These build and have been subjected to 0day testing, but might also need
testing by someone having the requisite hardware.

Thanx, Paul



 drivers/dax/super.c|   10 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c |5 +++
 drivers/gpu/drm/amd/amdkfd/kfd_process.c   |2 -
 drivers/gpu/drm/drm_drv.c  |8 
 include/linux/srcutree.h   |   19 +--
 kernel/rcu/rcuperf.c   |   40 +++-
 kernel/rcu/rcutorture.c|   48 +
 7 files changed, 105 insertions(+), 27 deletions(-)

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel