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

Message

Paul E. McKenney April 2, 2019, 2:28 p.m. UTC

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(-)

Comments

Mathieu Desnoyers April 2, 2019, 3:14 p.m. UTC | #1

----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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(-)

Paul E. McKenney April 2, 2019, 3:23 p.m. UTC | #2

On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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
>

Joel Fernandes April 2, 2019, 6:40 p.m. UTC | #3

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 [e-mail] 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

Paul E. McKenney April 3, 2019, 1:19 p.m. UTC | #4

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 [e-mail] 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/[e-mail]/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

Paul E. McKenney April 3, 2019, 1:32 p.m. UTC | #5

On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> 
> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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
>

Mathieu Desnoyers April 3, 2019, 2:27 p.m. UTC | #6

----- On Apr 3, 2019, at 9:32 AM, paulmck [e-mail] wrote:

> On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
>> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
>> 
>> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
>> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
        __TRACEPOINT_ENTRY(name);

And kernel/module.c:

find_module_sections():

#ifdef CONFIG_TRACEPOINTS
        mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
                                             sizeof(*mod->tracepoints_ptrs),
                                             &mod->num_tracepoints);
#endif

And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
notifier.

Basically you would want to have your own structure within your own section of
the module which describes the srcu domain, and have a module coming/going
notifier responsible for dynamically allocating the srcu domain on "coming", and
doing a srcu barrier and cleanup the domain on "going".

Thanks,

Mathieu


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

Paul E. McKenney April 3, 2019, 4:20 p.m. UTC | #7

On Wed, Apr 03, 2019 at 10:27:42AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 3, 2019, at 9:32 AM, paulmck [e-mail] wrote:
> 
> > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> >> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> >> 
> >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> >> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
>         __TRACEPOINT_ENTRY(name);
> 
> And kernel/module.c:
> 
> find_module_sections():
> 
> #ifdef CONFIG_TRACEPOINTS
>         mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
>                                              sizeof(*mod->tracepoints_ptrs),
>                                              &mod->num_tracepoints);
> #endif
> 
> And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
> notifier.
> 
> Basically you would want to have your own structure within your own section of
> the module which describes the srcu domain, and have a module coming/going
> notifier responsible for dynamically allocating the srcu domain on "coming", and
> doing a srcu barrier and cleanup the domain on "going".

Ah, sounds like an excellent approach!  I will give it a shot, thank you!

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> >							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
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
>

Joel Fernandes April 3, 2019, 7:30 p.m. UTC | #8

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 [e-mail] wrote:
> > 
> > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > >> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> > >> 
> > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > >> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
> >         __TRACEPOINT_ENTRY(name);
> > 
> > And kernel/module.c:
> > 
> > find_module_sections():
> > 
> > #ifdef CONFIG_TRACEPOINTS
> >         mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
> >                                              sizeof(*mod->tracepoints_ptrs),
> >                                              &mod->num_tracepoints);
> > #endif
> > 
> > And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
> > notifier.
> > 
> > Basically you would want to have your own structure within your own section of
> > the module which describes the srcu domain, and have a module coming/going
> > notifier responsible for dynamically allocating the srcu domain on "coming", and
> > doing a srcu barrier and cleanup the domain on "going".
> 
> Ah, sounds like an excellent approach!  I will give it a shot, thank you!

I agree with the idea as well. It is nice that tracepoints work with modules
so well, a feature that many folks use for debugging ;-)

thanks!

- Joel

Paul E. McKenney April 5, 2019, 11:28 p.m. UTC | #9

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 [e-mail] wrote:
> > 
> > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > >> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> > >> 
> > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > >> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
> >         __TRACEPOINT_ENTRY(name);
> > 
> > And kernel/module.c:
> > 
> > find_module_sections():
> > 
> > #ifdef CONFIG_TRACEPOINTS
> >         mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
> >                                              sizeof(*mod->tracepoints_ptrs),
> >                                              &mod->num_tracepoints);
> > #endif
> > 
> > And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
> > notifier.
> > 
> > Basically you would want to have your own structure within your own section of
> > the module which describes the srcu domain, and have a module coming/going
> > notifier responsible for dynamically allocating the srcu domain on "coming", and
> > doing a srcu barrier and cleanup the domain on "going".
> 
> Ah, sounds like an excellent approach!  I will give it a shot, thank you!

Please see below for an untested shot.

The original commits posted in this series are still available within
the -srcu tree at branch srcunomod.2019.04.05a.  Yes, I am a digital
packrat.  Why do you ask?

Thoughts?  Or more accurately, given that this is the first time I
have used linker sections, what did I mess up?

							Thanx, Paul

------------------------------------------------------------------------

commit e24a0dab1414c563bb96bcb28d5963c9df18b1e8
Author: Paul E. McKenney <[e-mail]>
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 <[e-mail]>
    Signed-off-by: Paul E. McKenney <[e-mail]>

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 <linux/rbtree_latch.h>
 #include <linux/error-injection.h>
 #include <linux/tracepoint-defs.h>
+#include <linux/srcu.h>
 
 #include <linux/percpu.h>
 #include <asm/module.h>
@@ -448,6 +449,10 @@ struct module {
 	unsigned int num_tracepoints;
 	tracepoint_ptr_t *tracepoints_ptrs;
 #endif
+#ifdef CONFIG_TREE_SRCU
+	unsigned int num_srcu_structs;
+	struct srcu_struct **srcu_struct_ptrs;
+#endif
 #ifdef CONFIG_BPF_EVENTS
 	unsigned int num_bpf_raw_events;
 	struct bpf_raw_event_map *bpf_raw_events;
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 7f7c8c050f63..93685a9f3b4c 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -120,9 +120,17 @@ struct srcu_struct {
  *
  * See include/linux/percpu-defs.h for the rules on per-CPU variables.
  */
-#define __DEFINE_SRCU(name, is_static)					\
-	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);\
-	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name##_srcu_data)
+#ifdef MODULE
+# define __DEFINE_SRCU(name, is_static)					\
+	is_static struct srcu_struct name;				\
+	struct srcu_struct *__srcu_struct_##name			\
+		__attribute__((section("___srcu_struct_ptrs"))) = &name
+#else
+# define __DEFINE_SRCU(name, is_static)					\
+	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);	\
+	is_static struct srcu_struct name =				\
+		__SRCU_STRUCT_INIT(name, name##_srcu_data)
+#endif
 #define DEFINE_SRCU(name)		__DEFINE_SRCU(name, /* not static */)
 #define DEFINE_STATIC_SRCU(name)	__DEFINE_SRCU(name, static)
 
diff --git a/kernel/module.c b/kernel/module.c
index 0b9aa8ab89f0..524da609c884 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3093,6 +3093,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 					     sizeof(*mod->tracepoints_ptrs),
 					     &mod->num_tracepoints);
 #endif
+#ifdef CONFIG_TREE_SRCU
+	mod->srcu_struct_ptrs = section_objs(info, "___srcu_struct_ptrs",
+					     sizeof(*mod->srcu_struct_ptrs),
+					     &mod->num_srcu_structs);
+#endif
 #ifdef CONFIG_BPF_EVENTS
 	mod->bpf_raw_events = section_objs(info, "__bpf_raw_tp_map",
 					   sizeof(*mod->bpf_raw_events),
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 9b761e546de8..ac0f6f0a8916 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1310,3 +1310,70 @@ void __init srcu_init(void)
 		queue_work(rcu_gp_wq, &ssp->work.work);
 	}
 }
+
+#ifdef CONFIG_MODULES
+
+/* Initialize any global-scope srcu_struct structures used by this module. */
+static int srcu_module_coming(struct module *mod)
+{
+	int i;
+	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
+	int ret;
+
+	for (i = 0; i < mod->num_srcu_structs; i++) {
+		ret = init_srcu_struct(*(sspp++));
+		if (WARN_ON_ONCE(ret))
+			return ret;
+	}
+	return 0;
+}
+
+/* Clean up any global-scope srcu_struct structures used by this module. */
+static void srcu_module_going(struct module *mod)
+{
+	int i;
+	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
+
+	for (i = 0; i < mod->num_srcu_structs; i++)
+		cleanup_srcu_struct(*(sspp++));
+}
+
+/* Handle one module, either coming or going. */
+static int srcu_module_notify(struct notifier_block *self,
+			      unsigned long val, void *data)
+{
+	struct module *mod = data;
+	int ret = 0;
+
+	switch (val) {
+	case MODULE_STATE_COMING:
+		ret = srcu_module_coming(mod);
+		break;
+	case MODULE_STATE_LIVE:
+		break;
+	case MODULE_STATE_GOING:
+		srcu_module_going(mod);
+		break;
+	case MODULE_STATE_UNFORMED:
+		break;
+	}
+	return ret;
+}
+
+static struct notifier_block srcu_module_nb = {
+	.notifier_call = srcu_module_notify,
+	.priority = 0,
+};
+
+static __init int init_srcu_module_notifier(void)
+{
+	int ret;
+
+	ret = register_module_notifier(&srcu_module_nb);
+	if (ret)
+		pr_warn("Failed to register srcu module notifier\n");
+	return ret;
+}
+late_initcall(init_srcu_module_notifier);
+
+#endif /* #ifdef CONFIG_MODULES */

Joel Fernandes April 6, 2019, 1:33 p.m. UTC | #10

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 [e-mail] wrote:
> > > 
> > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > >> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> > > >> 
> > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > >> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
> > >         __TRACEPOINT_ENTRY(name);
> > > 
> > > And kernel/module.c:
> > > 
> > > find_module_sections():
> > > 
> > > #ifdef CONFIG_TRACEPOINTS
> > >         mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
> > >                                              sizeof(*mod->tracepoints_ptrs),
> > >                                              &mod->num_tracepoints);
> > > #endif
> > > 
> > > And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
> > > notifier.
> > > 
> > > Basically you would want to have your own structure within your own section of
> > > the module which describes the srcu domain, and have a module coming/going
> > > notifier responsible for dynamically allocating the srcu domain on "coming", and
> > > doing a srcu barrier and cleanup the domain on "going".
> > 
> > Ah, sounds like an excellent approach!  I will give it a shot, thank you!
> 
> Please see below for an untested shot.
> 
> The original commits posted in this series are still available within
> the -srcu tree at branch srcunomod.2019.04.05a.  Yes, I am a digital
> packrat.  Why do you ask?
> 
> Thoughts?  Or more accurately, given that this is the first time I
> have used linker sections, what did I mess up?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit e24a0dab1414c563bb96bcb28d5963c9df18b1e8
> Author: Paul E. McKenney <[e-mail]>
> 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 <[e-mail]>
>     Signed-off-by: Paul E. McKenney <[e-mail]>
> 
> 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 <linux/rbtree_latch.h>
>  #include <linux/error-injection.h>
>  #include <linux/tracepoint-defs.h>
> +#include <linux/srcu.h>
>  
>  #include <linux/percpu.h>
>  #include <asm/module.h>
> @@ -448,6 +449,10 @@ struct module {
>  	unsigned int num_tracepoints;
>  	tracepoint_ptr_t *tracepoints_ptrs;
>  #endif
> +#ifdef CONFIG_TREE_SRCU
> +	unsigned int num_srcu_structs;
> +	struct srcu_struct **srcu_struct_ptrs;
> +#endif
>  #ifdef CONFIG_BPF_EVENTS
>  	unsigned int num_bpf_raw_events;
>  	struct bpf_raw_event_map *bpf_raw_events;
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index 7f7c8c050f63..93685a9f3b4c 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -120,9 +120,17 @@ struct srcu_struct {
>   *
>   * See include/linux/percpu-defs.h for the rules on per-CPU variables.
>   */
> -#define __DEFINE_SRCU(name, is_static)					\
> -	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);\
> -	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name##_srcu_data)
> +#ifdef MODULE
> +# define __DEFINE_SRCU(name, is_static)					\
> +	is_static struct srcu_struct name;				\
> +	struct srcu_struct *__srcu_struct_##name			\
> +		__attribute__((section("___srcu_struct_ptrs"))) = &name

This can be replaced by:
__section("__srcu_struct_ptrs") = &name;

> +#else
> +# define __DEFINE_SRCU(name, is_static)					\
> +	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);	\
> +	is_static struct srcu_struct name =				\
> +		__SRCU_STRUCT_INIT(name, name##_srcu_data)
> +#endif
>  #define DEFINE_SRCU(name)		__DEFINE_SRCU(name, /* not static */)
>  #define DEFINE_STATIC_SRCU(name)	__DEFINE_SRCU(name, static)
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index 0b9aa8ab89f0..524da609c884 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3093,6 +3093,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  					     sizeof(*mod->tracepoints_ptrs),
>  					     &mod->num_tracepoints);
>  #endif
> +#ifdef CONFIG_TREE_SRCU
> +	mod->srcu_struct_ptrs = section_objs(info, "___srcu_struct_ptrs",
> +					     sizeof(*mod->srcu_struct_ptrs),
> +					     &mod->num_srcu_structs);
> +#endif
>  #ifdef CONFIG_BPF_EVENTS
>  	mod->bpf_raw_events = section_objs(info, "__bpf_raw_tp_map",
>  					   sizeof(*mod->bpf_raw_events),
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 9b761e546de8..ac0f6f0a8916 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1310,3 +1310,70 @@ void __init srcu_init(void)
>  		queue_work(rcu_gp_wq, &ssp->work.work);
>  	}
>  }
> +
> +#ifdef CONFIG_MODULES
> +
> +/* Initialize any global-scope srcu_struct structures used by this module. */
> +static int srcu_module_coming(struct module *mod)
> +{
> +	int i;
> +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> +	int ret;
> +
> +	for (i = 0; i < mod->num_srcu_structs; i++) {
> +		ret = init_srcu_struct(*(sspp++));
> +		if (WARN_ON_ONCE(ret))
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/* Clean up any global-scope srcu_struct structures used by this module. */
> +static void srcu_module_going(struct module *mod)
> +{
> +	int i;
> +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> +
> +	for (i = 0; i < mod->num_srcu_structs; i++)
> +		cleanup_srcu_struct(*(sspp++));
> +}
> +
> +/* Handle one module, either coming or going. */
> +static int srcu_module_notify(struct notifier_block *self,
> +			      unsigned long val, void *data)
> +{
> +	struct module *mod = data;
> +	int ret = 0;
> +
> +	switch (val) {
> +	case MODULE_STATE_COMING:
> +		ret = srcu_module_coming(mod);
> +		break;
> +	case MODULE_STATE_LIVE:
> +		break;
> +	case MODULE_STATE_GOING:
> +		srcu_module_going(mod);
> +		break;
> +	case MODULE_STATE_UNFORMED:
> +		break;

The unused cases can be put under default or even clubbed thus saving a line
but if you prefer to keep them.like this that is fine.

It looks good to me but I will test it out more later today. Thanks!

- Joel


> +	return ret;
> +}
> +
> +static struct notifier_block srcu_module_nb = {
> +	.notifier_call = srcu_module_notify,
> +	.priority = 0,
> +};
> +
> +static __init int init_srcu_module_notifier(void)
> +{
> +	int ret;
> +
> +	ret = register_module_notifier(&srcu_module_nb);
> +	if (ret)
> +		pr_warn("Failed to register srcu module notifier\n");
> +	return ret;
> +}
> +late_initcall(init_srcu_module_notifier);
> +
> +#endif /* #ifdef CONFIG_MODULES */
>

Joel Fernandes April 6, 2019, 11:06 p.m. UTC | #11

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 [e-mail] wrote:
> > > 
> > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > >> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> > > >> 
> > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > >> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
> > >         __TRACEPOINT_ENTRY(name);
> > > 
> > > And kernel/module.c:
> > > 
> > > find_module_sections():
> > > 
> > > #ifdef CONFIG_TRACEPOINTS
> > >         mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
> > >                                              sizeof(*mod->tracepoints_ptrs),
> > >                                              &mod->num_tracepoints);
> > > #endif
> > > 
> > > And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
> > > notifier.
> > > 
> > > Basically you would want to have your own structure within your own section of
> > > the module which describes the srcu domain, and have a module coming/going
> > > notifier responsible for dynamically allocating the srcu domain on "coming", and
> > > doing a srcu barrier and cleanup the domain on "going".
> > 
> > Ah, sounds like an excellent approach!  I will give it a shot, thank you!
> 
> Please see below for an untested shot.
> 
> The original commits posted in this series are still available within
> the -srcu tree at branch srcunomod.2019.04.05a.  Yes, I am a digital
> packrat.  Why do you ask?
> 
> Thoughts?  Or more accurately, given that this is the first time I
> have used linker sections, what did I mess up?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit e24a0dab1414c563bb96bcb28d5963c9df18b1e8
> Author: Paul E. McKenney <[e-mail]>
> 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 <[e-mail]>
>     Signed-off-by: Paul E. McKenney <[e-mail]>
> 
> 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.

Thanks!

---8<-----------------------

From 369ad090f706ce8e1facdd18eb10828b5f7e2b72 Mon Sep 17 00:00:00 2001
From: "Joel Fernandes (Google)" <[e-mail]>
Date: Sat, 6 Apr 2019 18:57:17 -0400
Subject: [PATCH] srcu: Remove unused vmlinux srcu linker entries

The SRCU for modules optimization introduced vmlinux linker entries
which is unused since it applies only to the built-in vmlinux. So remove
it to prevent any space usage due to the 8 byte alignment.

Tested with SRCU torture_type and rcutorture.

Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Joel Fernandes (Google) <[e-mail]>
---
 include/asm-generic/vmlinux.lds.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index c2d919a1566e..f8f6f04c4453 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -338,10 +338,6 @@
 		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) {		\

Paul E. McKenney April 7, 2019, 1:39 p.m. UTC | #12

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 [e-mail] wrote:
> > > > 
> > > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > > >> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> > > > >> 
> > > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > > >> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
> > > >         __TRACEPOINT_ENTRY(name);
> > > > 
> > > > And kernel/module.c:
> > > > 
> > > > find_module_sections():
> > > > 
> > > > #ifdef CONFIG_TRACEPOINTS
> > > >         mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
> > > >                                              sizeof(*mod->tracepoints_ptrs),
> > > >                                              &mod->num_tracepoints);
> > > > #endif
> > > > 
> > > > And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
> > > > notifier.
> > > > 
> > > > Basically you would want to have your own structure within your own section of
> > > > the module which describes the srcu domain, and have a module coming/going
> > > > notifier responsible for dynamically allocating the srcu domain on "coming", and
> > > > doing a srcu barrier and cleanup the domain on "going".
> > > 
> > > Ah, sounds like an excellent approach!  I will give it a shot, thank you!
> > 
> > Please see below for an untested shot.
> > 
> > The original commits posted in this series are still available within
> > the -srcu tree at branch srcunomod.2019.04.05a.  Yes, I am a digital
> > packrat.  Why do you ask?
> > 
> > Thoughts?  Or more accurately, given that this is the first time I
> > have used linker sections, what did I mess up?
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit e24a0dab1414c563bb96bcb28d5963c9df18b1e8
> > Author: Paul E. McKenney <[e-mail]>
> > 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 <[e-mail]>
> >     Signed-off-by: Paul E. McKenney <[e-mail]>
> > 
> > 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?

								Thanx,Paul

> Thanks!
> 
> ---8<-----------------------
> 
> >From 369ad090f706ce8e1facdd18eb10828b5f7e2b72 Mon Sep 17 00:00:00 2001
> From: "Joel Fernandes (Google)" <[e-mail]>
> Date: Sat, 6 Apr 2019 18:57:17 -0400
> Subject: [PATCH] srcu: Remove unused vmlinux srcu linker entries
> 
> The SRCU for modules optimization introduced vmlinux linker entries
> which is unused since it applies only to the built-in vmlinux. So remove
> it to prevent any space usage due to the 8 byte alignment.
> 
> Tested with SRCU torture_type and rcutorture.
> 
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Joel Fernandes (Google) <[e-mail]>
> ---
>  include/asm-generic/vmlinux.lds.h | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index c2d919a1566e..f8f6f04c4453 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -338,10 +338,6 @@
>  		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) {		\
> -- 
> 2.21.0.392.gf8f6787159e-goog
>

Paul E. McKenney April 7, 2019, 1:48 p.m. UTC | #13

On Sat, Apr 06, 2019 at 01:33:27PM +0000, 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 [e-mail] wrote:
> > > > 
> > > > > On Tue, Apr 02, 2019 at 11:34:07AM -0400, Mathieu Desnoyers wrote:
> > > > >> ----- On Apr 2, 2019, at 11:23 AM, paulmck [e-mail] wrote:
> > > > >> 
> > > > >> > On Tue, Apr 02, 2019 at 11:14:40AM -0400, Mathieu Desnoyers wrote:
> > > > >> >> ----- On Apr 2, 2019, at 10:28 AM, paulmck [e-mail] 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 };\
> > > >         __TRACEPOINT_ENTRY(name);
> > > > 
> > > > And kernel/module.c:
> > > > 
> > > > find_module_sections():
> > > > 
> > > > #ifdef CONFIG_TRACEPOINTS
> > > >         mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
> > > >                                              sizeof(*mod->tracepoints_ptrs),
> > > >                                              &mod->num_tracepoints);
> > > > #endif
> > > > 
> > > > And kernel/tracepoint.c:tracepoint_module_notify() for the module coming/going
> > > > notifier.
> > > > 
> > > > Basically you would want to have your own structure within your own section of
> > > > the module which describes the srcu domain, and have a module coming/going
> > > > notifier responsible for dynamically allocating the srcu domain on "coming", and
> > > > doing a srcu barrier and cleanup the domain on "going".
> > > 
> > > Ah, sounds like an excellent approach!  I will give it a shot, thank you!
> > 
> > Please see below for an untested shot.
> > 
> > The original commits posted in this series are still available within
> > the -srcu tree at branch srcunomod.2019.04.05a.  Yes, I am a digital
> > packrat.  Why do you ask?
> > 
> > Thoughts?  Or more accurately, given that this is the first time I
> > have used linker sections, what did I mess up?
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit e24a0dab1414c563bb96bcb28d5963c9df18b1e8
> > Author: Paul E. McKenney <[e-mail]>
> > 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 <[e-mail]>
> >     Signed-off-by: Paul E. McKenney <[e-mail]>
> > 
> > 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 <linux/rbtree_latch.h>
> >  #include <linux/error-injection.h>
> >  #include <linux/tracepoint-defs.h>
> > +#include <linux/srcu.h>
> >  
> >  #include <linux/percpu.h>
> >  #include <asm/module.h>
> > @@ -448,6 +449,10 @@ struct module {
> >  	unsigned int num_tracepoints;
> >  	tracepoint_ptr_t *tracepoints_ptrs;
> >  #endif
> > +#ifdef CONFIG_TREE_SRCU
> > +	unsigned int num_srcu_structs;
> > +	struct srcu_struct **srcu_struct_ptrs;
> > +#endif
> >  #ifdef CONFIG_BPF_EVENTS
> >  	unsigned int num_bpf_raw_events;
> >  	struct bpf_raw_event_map *bpf_raw_events;
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index 7f7c8c050f63..93685a9f3b4c 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -120,9 +120,17 @@ struct srcu_struct {
> >   *
> >   * See include/linux/percpu-defs.h for the rules on per-CPU variables.
> >   */
> > -#define __DEFINE_SRCU(name, is_static)					\
> > -	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);\
> > -	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name##_srcu_data)
> > +#ifdef MODULE
> > +# define __DEFINE_SRCU(name, is_static)					\
> > +	is_static struct srcu_struct name;				\
> > +	struct srcu_struct *__srcu_struct_##name			\
> > +		__attribute__((section("___srcu_struct_ptrs"))) = &name
> 
> This can be replaced by:
> __section("__srcu_struct_ptrs") = &name;

That is way easier on the eyes!  Thank you, will fix.

> > +#else
> > +# define __DEFINE_SRCU(name, is_static)					\
> > +	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);	\
> > +	is_static struct srcu_struct name =				\
> > +		__SRCU_STRUCT_INIT(name, name##_srcu_data)
> > +#endif
> >  #define DEFINE_SRCU(name)		__DEFINE_SRCU(name, /* not static */)
> >  #define DEFINE_STATIC_SRCU(name)	__DEFINE_SRCU(name, static)
> >  
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 0b9aa8ab89f0..524da609c884 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3093,6 +3093,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> >  					     sizeof(*mod->tracepoints_ptrs),
> >  					     &mod->num_tracepoints);
> >  #endif
> > +#ifdef CONFIG_TREE_SRCU
> > +	mod->srcu_struct_ptrs = section_objs(info, "___srcu_struct_ptrs",
> > +					     sizeof(*mod->srcu_struct_ptrs),
> > +					     &mod->num_srcu_structs);
> > +#endif
> >  #ifdef CONFIG_BPF_EVENTS
> >  	mod->bpf_raw_events = section_objs(info, "__bpf_raw_tp_map",
> >  					   sizeof(*mod->bpf_raw_events),
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 9b761e546de8..ac0f6f0a8916 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -1310,3 +1310,70 @@ void __init srcu_init(void)
> >  		queue_work(rcu_gp_wq, &ssp->work.work);
> >  	}
> >  }
> > +
> > +#ifdef CONFIG_MODULES
> > +
> > +/* Initialize any global-scope srcu_struct structures used by this module. */
> > +static int srcu_module_coming(struct module *mod)
> > +{
> > +	int i;
> > +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> > +	int ret;
> > +
> > +	for (i = 0; i < mod->num_srcu_structs; i++) {
> > +		ret = init_srcu_struct(*(sspp++));
> > +		if (WARN_ON_ONCE(ret))
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/* Clean up any global-scope srcu_struct structures used by this module. */
> > +static void srcu_module_going(struct module *mod)
> > +{
> > +	int i;
> > +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> > +
> > +	for (i = 0; i < mod->num_srcu_structs; i++)
> > +		cleanup_srcu_struct(*(sspp++));
> > +}
> > +
> > +/* Handle one module, either coming or going. */
> > +static int srcu_module_notify(struct notifier_block *self,
> > +			      unsigned long val, void *data)
> > +{
> > +	struct module *mod = data;
> > +	int ret = 0;
> > +
> > +	switch (val) {
> > +	case MODULE_STATE_COMING:
> > +		ret = srcu_module_coming(mod);
> > +		break;
> > +	case MODULE_STATE_LIVE:
> > +		break;
> > +	case MODULE_STATE_GOING:
> > +		srcu_module_going(mod);
> > +		break;
> > +	case MODULE_STATE_UNFORMED:
> > +		break;
> 
> The unused cases can be put under default or even clubbed thus saving a line
> but if you prefer to keep them.like this that is fine.

Well, saving two lines is saving two lines, so will apply.

> It looks good to me but I will test it out more later today. Thanks!

Thank you!

							Thanx, Paul

> - Joel
> 
> 
> > +	return ret;
> > +}
> > +
> > +static struct notifier_block srcu_module_nb = {
> > +	.notifier_call = srcu_module_notify,
> > +	.priority = 0,
> > +};
> > +
> > +static __init int init_srcu_module_notifier(void)
> > +{
> > +	int ret;
> > +
> > +	ret = register_module_notifier(&srcu_module_nb);
> > +	if (ret)
> > +		pr_warn("Failed to register srcu module notifier\n");
> > +	return ret;
> > +}
> > +late_initcall(init_srcu_module_notifier);
> > +
> > +#endif /* #ifdef CONFIG_MODULES */
> > 
>

Paul E. McKenney April 7, 2019, 1:59 p.m. UTC | #14

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 <[e-mail]>
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 <[e-mail]>
    [ paulmck: Use __section() and use "default" in srcu_module_notify()'s
      "switch" statement as suggested by Joel Fernandes. ]
    Signed-off-by: Paul E. McKenney <[e-mail]>

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 <linux/rbtree_latch.h>
 #include <linux/error-injection.h>
 #include <linux/tracepoint-defs.h>
+#include <linux/srcu.h>
 
 #include <linux/percpu.h>
 #include <asm/module.h>
@@ -448,6 +449,10 @@ struct module {
 	unsigned int num_tracepoints;
 	tracepoint_ptr_t *tracepoints_ptrs;
 #endif
+#ifdef CONFIG_TREE_SRCU
+	unsigned int num_srcu_structs;
+	struct srcu_struct **srcu_struct_ptrs;
+#endif
 #ifdef CONFIG_BPF_EVENTS
 	unsigned int num_bpf_raw_events;
 	struct bpf_raw_event_map *bpf_raw_events;
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 7f7c8c050f63..8af1824c46a8 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -120,9 +120,17 @@ struct srcu_struct {
  *
  * See include/linux/percpu-defs.h for the rules on per-CPU variables.
  */
-#define __DEFINE_SRCU(name, is_static)					\
-	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);\
-	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name##_srcu_data)
+#ifdef MODULE
+# define __DEFINE_SRCU(name, is_static)					\
+	is_static struct srcu_struct name;				\
+	struct srcu_struct *__srcu_struct_##name			\
+		__section("___srcu_struct_ptrs") = &name
+#else
+# define __DEFINE_SRCU(name, is_static)					\
+	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);	\
+	is_static struct srcu_struct name =				\
+		__SRCU_STRUCT_INIT(name, name##_srcu_data)
+#endif
 #define DEFINE_SRCU(name)		__DEFINE_SRCU(name, /* not static */)
 #define DEFINE_STATIC_SRCU(name)	__DEFINE_SRCU(name, static)
 
diff --git a/kernel/module.c b/kernel/module.c
index 0b9aa8ab89f0..524da609c884 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3093,6 +3093,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 					     sizeof(*mod->tracepoints_ptrs),
 					     &mod->num_tracepoints);
 #endif
+#ifdef CONFIG_TREE_SRCU
+	mod->srcu_struct_ptrs = section_objs(info, "___srcu_struct_ptrs",
+					     sizeof(*mod->srcu_struct_ptrs),
+					     &mod->num_srcu_structs);
+#endif
 #ifdef CONFIG_BPF_EVENTS
 	mod->bpf_raw_events = section_objs(info, "__bpf_raw_tp_map",
 					   sizeof(*mod->bpf_raw_events),
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 9b761e546de8..2ded2614a2f4 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1310,3 +1310,68 @@ void __init srcu_init(void)
 		queue_work(rcu_gp_wq, &ssp->work.work);
 	}
 }
+
+#ifdef CONFIG_MODULES
+
+/* Initialize any global-scope srcu_struct structures used by this module. */
+static int srcu_module_coming(struct module *mod)
+{
+	int i;
+	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
+	int ret;
+
+	for (i = 0; i < mod->num_srcu_structs; i++) {
+		ret = init_srcu_struct(*(sspp++));
+		if (WARN_ON_ONCE(ret))
+			return ret;
+	}
+	return 0;
+}
+
+/* Clean up any global-scope srcu_struct structures used by this module. */
+static void srcu_module_going(struct module *mod)
+{
+	int i;
+	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
+
+	for (i = 0; i < mod->num_srcu_structs; i++)
+		cleanup_srcu_struct(*(sspp++));
+}
+
+/* Handle one module, either coming or going. */
+static int srcu_module_notify(struct notifier_block *self,
+			      unsigned long val, void *data)
+{
+	struct module *mod = data;
+	int ret = 0;
+
+	switch (val) {
+	case MODULE_STATE_COMING:
+		ret = srcu_module_coming(mod);
+		break;
+	case MODULE_STATE_GOING:
+		srcu_module_going(mod);
+		break;
+	default:
+		break;
+	}
+	return ret;
+}
+
+static struct notifier_block srcu_module_nb = {
+	.notifier_call = srcu_module_notify,
+	.priority = 0,
+};
+
+static __init int init_srcu_module_notifier(void)
+{
+	int ret;
+
+	ret = register_module_notifier(&srcu_module_nb);
+	if (ret)
+		pr_warn("Failed to register srcu module notifier\n");
+	return ret;
+}
+late_initcall(init_srcu_module_notifier);
+
+#endif /* #ifdef CONFIG_MODULES */

Joel Fernandes April 7, 2019, 3:46 p.m. UTC | #15

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 <[e-mail]>
> 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 <[e-mail]>
>     [ paulmck: Use __section() and use "default" in srcu_module_notify()'s
>       "switch" statement as suggested by Joel Fernandes. ]
>     Signed-off-by: Paul E. McKenney <[e-mail]>
> 
> 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 <linux/rbtree_latch.h>
>  #include <linux/error-injection.h>
>  #include <linux/tracepoint-defs.h>
> +#include <linux/srcu.h>
>  
>  #include <linux/percpu.h>
>  #include <asm/module.h>
> @@ -448,6 +449,10 @@ struct module {
>  	unsigned int num_tracepoints;
>  	tracepoint_ptr_t *tracepoints_ptrs;
>  #endif
> +#ifdef CONFIG_TREE_SRCU
> +	unsigned int num_srcu_structs;
> +	struct srcu_struct **srcu_struct_ptrs;
> +#endif
>  #ifdef CONFIG_BPF_EVENTS
>  	unsigned int num_bpf_raw_events;
>  	struct bpf_raw_event_map *bpf_raw_events;
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index 7f7c8c050f63..8af1824c46a8 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -120,9 +120,17 @@ struct srcu_struct {
>   *
>   * See include/linux/percpu-defs.h for the rules on per-CPU variables.
>   */
> -#define __DEFINE_SRCU(name, is_static)					\
> -	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);\
> -	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name##_srcu_data)
> +#ifdef MODULE
> +# define __DEFINE_SRCU(name, is_static)					\
> +	is_static struct srcu_struct name;				\
> +	struct srcu_struct *__srcu_struct_##name			\
> +		__section("___srcu_struct_ptrs") = &name
> +#else
> +# define __DEFINE_SRCU(name, is_static)					\
> +	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);	\
> +	is_static struct srcu_struct name =				\
> +		__SRCU_STRUCT_INIT(name, name##_srcu_data)
> +#endif
>  #define DEFINE_SRCU(name)		__DEFINE_SRCU(name, /* not static */)
>  #define DEFINE_STATIC_SRCU(name)	__DEFINE_SRCU(name, static)
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index 0b9aa8ab89f0..524da609c884 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3093,6 +3093,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  					     sizeof(*mod->tracepoints_ptrs),
>  					     &mod->num_tracepoints);
>  #endif
> +#ifdef CONFIG_TREE_SRCU
> +	mod->srcu_struct_ptrs = section_objs(info, "___srcu_struct_ptrs",
> +					     sizeof(*mod->srcu_struct_ptrs),
> +					     &mod->num_srcu_structs);
> +#endif
>  #ifdef CONFIG_BPF_EVENTS
>  	mod->bpf_raw_events = section_objs(info, "__bpf_raw_tp_map",
>  					   sizeof(*mod->bpf_raw_events),
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 9b761e546de8..2ded2614a2f4 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1310,3 +1310,68 @@ void __init srcu_init(void)
>  		queue_work(rcu_gp_wq, &ssp->work.work);
>  	}
>  }
> +
> +#ifdef CONFIG_MODULES
> +
> +/* Initialize any global-scope srcu_struct structures used by this module. */
> +static int srcu_module_coming(struct module *mod)
> +{
> +	int i;
> +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> +	int ret;
> +
> +	for (i = 0; i < mod->num_srcu_structs; i++) {
> +		ret = init_srcu_struct(*(sspp++));
> +		if (WARN_ON_ONCE(ret))
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/* Clean up any global-scope srcu_struct structures used by this module. */
> +static void srcu_module_going(struct module *mod)
> +{
> +	int i;
> +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> +
> +	for (i = 0; i < mod->num_srcu_structs; i++)
> +		cleanup_srcu_struct(*(sspp++));
> +}
> +
> +/* Handle one module, either coming or going. */
> +static int srcu_module_notify(struct notifier_block *self,
> +			      unsigned long val, void *data)
> +{
> +	struct module *mod = data;
> +	int ret = 0;
> +
> +	switch (val) {
> +	case MODULE_STATE_COMING:
> +		ret = srcu_module_coming(mod);
> +		break;
> +	case MODULE_STATE_GOING:
> +		srcu_module_going(mod);
> +		break;
> +	default:
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static struct notifier_block srcu_module_nb = {
> +	.notifier_call = srcu_module_notify,
> +	.priority = 0,
> +};
> +
> +static __init int init_srcu_module_notifier(void)
> +{
> +	int ret;
> +
> +	ret = register_module_notifier(&srcu_module_nb);
> +	if (ret)
> +		pr_warn("Failed to register srcu module notifier\n");
> +	return ret;
> +}
> +late_initcall(init_srcu_module_notifier);
> +
> +#endif /* #ifdef CONFIG_MODULES */
>

Paul E. McKenney April 7, 2019, 5:05 p.m. UTC | #16

On Sun, Apr 07, 2019 at 03:46:13PM +0000, 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 <[e-mail]>
> > 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 <[e-mail]>
> >     [ paulmck: Use __section() and use "default" in srcu_module_notify()'s
> >       "switch" statement as suggested by Joel Fernandes. ]
> >     Signed-off-by: Paul E. McKenney <[e-mail]>
> > 
> > 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 <linux/rbtree_latch.h>
> >  #include <linux/error-injection.h>
> >  #include <linux/tracepoint-defs.h>
> > +#include <linux/srcu.h>
> >  
> >  #include <linux/percpu.h>
> >  #include <asm/module.h>
> > @@ -448,6 +449,10 @@ struct module {
> >  	unsigned int num_tracepoints;
> >  	tracepoint_ptr_t *tracepoints_ptrs;
> >  #endif
> > +#ifdef CONFIG_TREE_SRCU
> > +	unsigned int num_srcu_structs;
> > +	struct srcu_struct **srcu_struct_ptrs;
> > +#endif
> >  #ifdef CONFIG_BPF_EVENTS
> >  	unsigned int num_bpf_raw_events;
> >  	struct bpf_raw_event_map *bpf_raw_events;
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index 7f7c8c050f63..8af1824c46a8 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -120,9 +120,17 @@ struct srcu_struct {
> >   *
> >   * See include/linux/percpu-defs.h for the rules on per-CPU variables.
> >   */
> > -#define __DEFINE_SRCU(name, is_static)					\
> > -	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);\
> > -	is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name##_srcu_data)
> > +#ifdef MODULE
> > +# define __DEFINE_SRCU(name, is_static)					\
> > +	is_static struct srcu_struct name;				\
> > +	struct srcu_struct *__srcu_struct_##name			\
> > +		__section("___srcu_struct_ptrs") = &name
> > +#else
> > +# define __DEFINE_SRCU(name, is_static)					\
> > +	static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);	\
> > +	is_static struct srcu_struct name =				\
> > +		__SRCU_STRUCT_INIT(name, name##_srcu_data)
> > +#endif
> >  #define DEFINE_SRCU(name)		__DEFINE_SRCU(name, /* not static */)
> >  #define DEFINE_STATIC_SRCU(name)	__DEFINE_SRCU(name, static)
> >  
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 0b9aa8ab89f0..524da609c884 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3093,6 +3093,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> >  					     sizeof(*mod->tracepoints_ptrs),
> >  					     &mod->num_tracepoints);
> >  #endif
> > +#ifdef CONFIG_TREE_SRCU
> > +	mod->srcu_struct_ptrs = section_objs(info, "___srcu_struct_ptrs",
> > +					     sizeof(*mod->srcu_struct_ptrs),
> > +					     &mod->num_srcu_structs);
> > +#endif
> >  #ifdef CONFIG_BPF_EVENTS
> >  	mod->bpf_raw_events = section_objs(info, "__bpf_raw_tp_map",
> >  					   sizeof(*mod->bpf_raw_events),
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 9b761e546de8..2ded2614a2f4 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -1310,3 +1310,68 @@ void __init srcu_init(void)
> >  		queue_work(rcu_gp_wq, &ssp->work.work);
> >  	}
> >  }
> > +
> > +#ifdef CONFIG_MODULES
> > +
> > +/* Initialize any global-scope srcu_struct structures used by this module. */
> > +static int srcu_module_coming(struct module *mod)
> > +{
> > +	int i;
> > +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> > +	int ret;
> > +
> > +	for (i = 0; i < mod->num_srcu_structs; i++) {
> > +		ret = init_srcu_struct(*(sspp++));
> > +		if (WARN_ON_ONCE(ret))
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/* Clean up any global-scope srcu_struct structures used by this module. */
> > +static void srcu_module_going(struct module *mod)
> > +{
> > +	int i;
> > +	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> > +
> > +	for (i = 0; i < mod->num_srcu_structs; i++)
> > +		cleanup_srcu_struct(*(sspp++));
> > +}
> > +
> > +/* Handle one module, either coming or going. */
> > +static int srcu_module_notify(struct notifier_block *self,
> > +			      unsigned long val, void *data)
> > +{
> > +	struct module *mod = data;
> > +	int ret = 0;
> > +
> > +	switch (val) {
> > +	case MODULE_STATE_COMING:
> > +		ret = srcu_module_coming(mod);
> > +		break;
> > +	case MODULE_STATE_GOING:
> > +		srcu_module_going(mod);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	return ret;
> > +}
> > +
> > +static struct notifier_block srcu_module_nb = {
> > +	.notifier_call = srcu_module_notify,
> > +	.priority = 0,
> > +};
> > +
> > +static __init int init_srcu_module_notifier(void)
> > +{
> > +	int ret;
> > +
> > +	ret = register_module_notifier(&srcu_module_nb);
> > +	if (ret)
> > +		pr_warn("Failed to register srcu module notifier\n");
> > +	return ret;
> > +}
> > +late_initcall(init_srcu_module_notifier);
> > +
> > +#endif /* #ifdef CONFIG_MODULES */
> > 
>

Mathieu Desnoyers April 7, 2019, 7:26 p.m. UTC | #17

----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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

Joel Fernandes April 7, 2019, 7:32 p.m. UTC | #18

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

Mathieu Desnoyers April 7, 2019, 8:41 p.m. UTC | #19

----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail] wrote:

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

Joel Fernandes April 7, 2019, 9:07 p.m. UTC | #20

On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> 
> ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail] wrote:
> 
> > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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

Joel Fernandes April 8, 2019, 12:36 a.m. UTC | #21

On Sun, Apr 07, 2019 at 10:05:14AM -0700, Paul E. McKenney wrote:
> On Sun, Apr 07, 2019 at 03:46:13PM +0000, 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

Paul E. McKenney April 8, 2019, 2:27 a.m. UTC | #22

On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> > 
> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail] wrote:
> > 
> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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

Paul E. McKenney April 8, 2019, 2:28 a.m. UTC | #23

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 +0000, 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, and thank you for giving it a go!
> 
> You're very welcome. thanks,
> 
>  - Joel
>

Mathieu Desnoyers April 8, 2019, 1:05 p.m. UTC | #24

----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:

> On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
>> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
>> > 
>> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
>> > wrote:
>> > 
>> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
>> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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

Paul E. McKenney April 8, 2019, 2:22 p.m. UTC | #25

On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
> 
> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> >> > 
> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
> >> > wrote:
> >> > 
> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.

So please let me try again.

If a given srcu_struct in a module never has call_srcu() invoked, there
is no need to invoke rcu_barrier() at any time, whether at module-unload
time or not.  Adding rcu_barrier() in this case adds overhead and latency
for no good reason.

If a given srcu_struct in a module does have at least one call_srcu()
invoked, it is already that module's responsibility to make sure that
the code sticks around long enough for the callback to be invoked.

This means that correct SRCU users that invoke call_srcu() already
have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
reasonable probability, now get a WARN_ON() at module-unload time, with
the per-CPU state getting leaked.  Before this change, they would (also
with reasonable probability) instead get an instruction-fetch fault when
the SRCU callback was invoked after the completion of the module unload.
Furthermore, in all cases where they would previously have gotten the
instruction-fetch fault, they now get the WARN_ON(), like this:

	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
		return; /* Forgot srcu_barrier(), so just leak it! */

So this change already represents an improvement in usability.

							Thanx, Paul

Mathieu Desnoyers April 8, 2019, 2:49 p.m. UTC | #26

----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:

> On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
>> 
>> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
>> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
>> >> > 
>> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
>> >> > wrote:
>> >> > 
>> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
>> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
> 
> So please let me try again.
> 
> If a given srcu_struct in a module never has call_srcu() invoked, there
> is no need to invoke rcu_barrier() at any time, whether at module-unload
> time or not.  Adding rcu_barrier() in this case adds overhead and latency
> for no good reason.

Not if we invoke srcu_barrier() for that specific domain. If
call_srcu was never invoked for a srcu domain, I don't see why
srcu_barrier() should be more expensive than a simple check that
the domain does not have any srcu work queued.

> 
> If a given srcu_struct in a module does have at least one call_srcu()
> invoked, it is already that module's responsibility to make sure that
> the code sticks around long enough for the callback to be invoked.

I understand that when users do explicit dynamic allocation/cleanup of
srcu domains, they indeed need to take care of doing explicit srcu_barrier().
However, if they do static definition of srcu domains, it would be nice
if we can handle the barriers under the hood.

> 
> This means that correct SRCU users that invoke call_srcu() already
> have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
> reasonable probability, now get a WARN_ON() at module-unload time, with
> the per-CPU state getting leaked.  Before this change, they would (also
> with reasonable probability) instead get an instruction-fetch fault when
> the SRCU callback was invoked after the completion of the module unload.
> Furthermore, in all cases where they would previously have gotten the
> instruction-fetch fault, they now get the WARN_ON(), like this:
> 
>	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
>		return; /* Forgot srcu_barrier(), so just leak it! */
> 
> So this change already represents an improvement in usability.

Considering that we can do a srcu_barrier() for the specific domain,
and that it should add no noticeable overhead if there is no queued
callbacks, I don't see a good reason for leaving the srcu_barrier
invocation to the user rather than implicitly doing it from the
module going notifier.

Thanks,

Mathieu

Paul E. McKenney April 8, 2019, 3:46 p.m. UTC | #27

On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
> 
> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
> >> 
> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> >> >> > 
> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
> >> >> > wrote:
> >> >> > 
> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
> > 
> > So please let me try again.
> > 
> > If a given srcu_struct in a module never has call_srcu() invoked, there
> > is no need to invoke rcu_barrier() at any time, whether at module-unload
> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
> > for no good reason.
> 
> Not if we invoke srcu_barrier() for that specific domain. If
> call_srcu was never invoked for a srcu domain, I don't see why
> srcu_barrier() should be more expensive than a simple check that
> the domain does not have any srcu work queued.

But that simple check does involve a cache miss for each possible CPU (not
just each online CPU), so it is non-trivial, especially on large systems.

> > If a given srcu_struct in a module does have at least one call_srcu()
> > invoked, it is already that module's responsibility to make sure that
> > the code sticks around long enough for the callback to be invoked.
> 
> I understand that when users do explicit dynamic allocation/cleanup of
> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
> However, if they do static definition of srcu domains, it would be nice
> if we can handle the barriers under the hood.

All else being equal, of course.  But...

> > This means that correct SRCU users that invoke call_srcu() already
> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
> > reasonable probability, now get a WARN_ON() at module-unload time, with
> > the per-CPU state getting leaked.  Before this change, they would (also
> > with reasonable probability) instead get an instruction-fetch fault when
> > the SRCU callback was invoked after the completion of the module unload.
> > Furthermore, in all cases where they would previously have gotten the
> > instruction-fetch fault, they now get the WARN_ON(), like this:
> > 
> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
> >		return; /* Forgot srcu_barrier(), so just leak it! */
> > 
> > So this change already represents an improvement in usability.
> 
> Considering that we can do a srcu_barrier() for the specific domain,
> and that it should add no noticeable overhead if there is no queued
> callbacks, I don't see a good reason for leaving the srcu_barrier
> invocation to the user rather than implicitly doing it from the
> module going notifier.

Now, I could automatically add an indicator of whether or not a
call_srcu() had happened, but then again, that would either add a
call_srcu() scalability bottleneck or again require a scan of all possible
CPUs...  to figure out if it was necessary to scan all possible CPUs.

Or is scanning all possible CPUs down in the noise in this case?  Or
am I missing a trick that would reduce the overhead?

							Thanx, Paul

Mathieu Desnoyers April 8, 2019, 5:24 p.m. UTC | #28

----- On Apr 8, 2019, at 11:46 AM, paulmck [e-mail] wrote:

> On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
>> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
>> 
>> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
>> >> 
>> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
>> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
>> >> >> > 
>> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
>> >> >> > wrote:
>> >> >> > 
>> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
>> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
>> > 
>> > So please let me try again.
>> > 
>> > If a given srcu_struct in a module never has call_srcu() invoked, there
>> > is no need to invoke rcu_barrier() at any time, whether at module-unload
>> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
>> > for no good reason.
>> 
>> Not if we invoke srcu_barrier() for that specific domain. If
>> call_srcu was never invoked for a srcu domain, I don't see why
>> srcu_barrier() should be more expensive than a simple check that
>> the domain does not have any srcu work queued.
> 
> But that simple check does involve a cache miss for each possible CPU (not
> just each online CPU), so it is non-trivial, especially on large systems.
> 
>> > If a given srcu_struct in a module does have at least one call_srcu()
>> > invoked, it is already that module's responsibility to make sure that
>> > the code sticks around long enough for the callback to be invoked.
>> 
>> I understand that when users do explicit dynamic allocation/cleanup of
>> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
>> However, if they do static definition of srcu domains, it would be nice
>> if we can handle the barriers under the hood.
> 
> All else being equal, of course.  But...
> 
>> > This means that correct SRCU users that invoke call_srcu() already
>> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
>> > reasonable probability, now get a WARN_ON() at module-unload time, with
>> > the per-CPU state getting leaked.  Before this change, they would (also
>> > with reasonable probability) instead get an instruction-fetch fault when
>> > the SRCU callback was invoked after the completion of the module unload.
>> > Furthermore, in all cases where they would previously have gotten the
>> > instruction-fetch fault, they now get the WARN_ON(), like this:
>> > 
>> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
>> >		return; /* Forgot srcu_barrier(), so just leak it! */
>> > 
>> > So this change already represents an improvement in usability.
>> 
>> Considering that we can do a srcu_barrier() for the specific domain,
>> and that it should add no noticeable overhead if there is no queued
>> callbacks, I don't see a good reason for leaving the srcu_barrier
>> invocation to the user rather than implicitly doing it from the
>> module going notifier.
> 
> Now, I could automatically add an indicator of whether or not a
> call_srcu() had happened, but then again, that would either add a
> call_srcu() scalability bottleneck or again require a scan of all possible
> CPUs...  to figure out if it was necessary to scan all possible CPUs.
> 
> Or is scanning all possible CPUs down in the noise in this case?  Or
> am I missing a trick that would reduce the overhead?

Module unloading implicitly does a synchronize_rcu (for RCU-sched), and
a stop_machine. So I would be tempted to say that overhead of iteration
over all CPUs might not matter that much considering the rest.

About notifying that a call_srcu has happened for the srcu domain in a
scalable fashion, let's see... We could have a flag "call_srcu_used"
for each call_srcu domain. Whenever call_srcu is invoked, it would
load that flag. It sets it on first use.

The idea here is to only use that flag when srcu_barrier is performed
right before the srcu domain cleanup (it could become part of that
cleanup). Else, using it in all srcu_barrier() might be tricky, because
we may then need to add memory barriers or locking to the call_srcu
fast-path, which is an overhead we try to avoid.

However, if we only use that flag as part of the srcu domain cleanup,
it's already prohibited to invoke call_srcu concurrently with the
cleanup of the same domain, so I don't think we would need any
memory barriers in call_srcu.

Thoughts ?

Thanks,

Mathieu

Joel Fernandes April 9, 2019, 3:40 p.m. UTC | #29

On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 8, 2019, at 11:46 AM, paulmck [e-mail] wrote:
> 
> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
> >> >> 
> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
> >> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> >> >> >> > 
> >> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
> >> >> >> > wrote:
> >> >> >> > 
> >> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> >> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
> >> > 
> >> > So please let me try again.
> >> > 
> >> > If a given srcu_struct in a module never has call_srcu() invoked, there
> >> > is no need to invoke rcu_barrier() at any time, whether at module-unload
> >> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
> >> > for no good reason.
> >> 
> >> Not if we invoke srcu_barrier() for that specific domain. If
> >> call_srcu was never invoked for a srcu domain, I don't see why
> >> srcu_barrier() should be more expensive than a simple check that
> >> the domain does not have any srcu work queued.
> > 
> > But that simple check does involve a cache miss for each possible CPU (not
> > just each online CPU), so it is non-trivial, especially on large systems.
> > 
> >> > If a given srcu_struct in a module does have at least one call_srcu()
> >> > invoked, it is already that module's responsibility to make sure that
> >> > the code sticks around long enough for the callback to be invoked.
> >> 
> >> I understand that when users do explicit dynamic allocation/cleanup of
> >> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
> >> However, if they do static definition of srcu domains, it would be nice
> >> if we can handle the barriers under the hood.
> > 
> > All else being equal, of course.  But...
> > 
> >> > This means that correct SRCU users that invoke call_srcu() already
> >> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
> >> > reasonable probability, now get a WARN_ON() at module-unload time, with
> >> > the per-CPU state getting leaked.  Before this change, they would (also
> >> > with reasonable probability) instead get an instruction-fetch fault when
> >> > the SRCU callback was invoked after the completion of the module unload.
> >> > Furthermore, in all cases where they would previously have gotten the
> >> > instruction-fetch fault, they now get the WARN_ON(), like this:
> >> > 
> >> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
> >> >		return; /* Forgot srcu_barrier(), so just leak it! */
> >> > 
> >> > So this change already represents an improvement in usability.
> >> 
> >> Considering that we can do a srcu_barrier() for the specific domain,
> >> and that it should add no noticeable overhead if there is no queued
> >> callbacks, I don't see a good reason for leaving the srcu_barrier
> >> invocation to the user rather than implicitly doing it from the
> >> module going notifier.
> > 
> > Now, I could automatically add an indicator of whether or not a
> > call_srcu() had happened, but then again, that would either add a
> > call_srcu() scalability bottleneck or again require a scan of all possible
> > CPUs...  to figure out if it was necessary to scan all possible CPUs.
> > 
> > Or is scanning all possible CPUs down in the noise in this case?  Or
> > am I missing a trick that would reduce the overhead?
> 
> Module unloading implicitly does a synchronize_rcu (for RCU-sched), and
> a stop_machine. So I would be tempted to say that overhead of iteration
> over all CPUs might not matter that much considering the rest.
> 
> About notifying that a call_srcu has happened for the srcu domain in a
> scalable fashion, let's see... We could have a flag "call_srcu_used"
> for each call_srcu domain. Whenever call_srcu is invoked, it would
> load that flag. It sets it on first use.
> 
> The idea here is to only use that flag when srcu_barrier is performed
> right before the srcu domain cleanup (it could become part of that
> cleanup). Else, using it in all srcu_barrier() might be tricky, because
> we may then need to add memory barriers or locking to the call_srcu
> fast-path, which is an overhead we try to avoid.
> 
> However, if we only use that flag as part of the srcu domain cleanup,
> it's already prohibited to invoke call_srcu concurrently with the
> cleanup of the same domain, so I don't think we would need any
> memory barriers in call_srcu.

About the last part of your email, it seems to that if after call_srcu has
returned, if the module could be unloaded on some other CPU - then it would
need to see the flag stored by the preceding call_srcu, so I believe there
would be a memory barrier between the two opreations (call_srcu and module
unload).

Also about doing the unconditional srcu_barrier, since a module could be
unloaded at any time - don't all SRCU using modules need to invoke
srcu_barrier() during their clean up anyway so we are incurring the barrier
overhead anyway? Or, am I missing a design pattern here? It seems to me
rcutorture module definitely calls srcu_barrier() before it is unloaded.

thanks,

 - Joel

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

Mathieu Desnoyers April 9, 2019, 3:56 p.m. UTC | #30

----- On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google [e-mail] wrote:

> On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
>> ----- On Apr 8, 2019, at 11:46 AM, paulmck [e-mail] wrote:
>> 
>> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
>> >> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
>> >> 
>> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> >> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
>> >> >> 
>> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
>> >> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
>> >> >> >> > 
>> >> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
>> >> >> >> > wrote:
>> >> >> >> > 
>> >> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
>> >> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
>> >> > 
>> >> > So please let me try again.
>> >> > 
>> >> > If a given srcu_struct in a module never has call_srcu() invoked, there
>> >> > is no need to invoke rcu_barrier() at any time, whether at module-unload
>> >> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
>> >> > for no good reason.
>> >> 
>> >> Not if we invoke srcu_barrier() for that specific domain. If
>> >> call_srcu was never invoked for a srcu domain, I don't see why
>> >> srcu_barrier() should be more expensive than a simple check that
>> >> the domain does not have any srcu work queued.
>> > 
>> > But that simple check does involve a cache miss for each possible CPU (not
>> > just each online CPU), so it is non-trivial, especially on large systems.
>> > 
>> >> > If a given srcu_struct in a module does have at least one call_srcu()
>> >> > invoked, it is already that module's responsibility to make sure that
>> >> > the code sticks around long enough for the callback to be invoked.
>> >> 
>> >> I understand that when users do explicit dynamic allocation/cleanup of
>> >> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
>> >> However, if they do static definition of srcu domains, it would be nice
>> >> if we can handle the barriers under the hood.
>> > 
>> > All else being equal, of course.  But...
>> > 
>> >> > This means that correct SRCU users that invoke call_srcu() already
>> >> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
>> >> > reasonable probability, now get a WARN_ON() at module-unload time, with
>> >> > the per-CPU state getting leaked.  Before this change, they would (also
>> >> > with reasonable probability) instead get an instruction-fetch fault when
>> >> > the SRCU callback was invoked after the completion of the module unload.
>> >> > Furthermore, in all cases where they would previously have gotten the
>> >> > instruction-fetch fault, they now get the WARN_ON(), like this:
>> >> > 
>> >> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
>> >> >		return; /* Forgot srcu_barrier(), so just leak it! */
>> >> > 
>> >> > So this change already represents an improvement in usability.
>> >> 
>> >> Considering that we can do a srcu_barrier() for the specific domain,
>> >> and that it should add no noticeable overhead if there is no queued
>> >> callbacks, I don't see a good reason for leaving the srcu_barrier
>> >> invocation to the user rather than implicitly doing it from the
>> >> module going notifier.
>> > 
>> > Now, I could automatically add an indicator of whether or not a
>> > call_srcu() had happened, but then again, that would either add a
>> > call_srcu() scalability bottleneck or again require a scan of all possible
>> > CPUs...  to figure out if it was necessary to scan all possible CPUs.
>> > 
>> > Or is scanning all possible CPUs down in the noise in this case?  Or
>> > am I missing a trick that would reduce the overhead?
>> 
>> Module unloading implicitly does a synchronize_rcu (for RCU-sched), and
>> a stop_machine. So I would be tempted to say that overhead of iteration
>> over all CPUs might not matter that much considering the rest.
>> 
>> About notifying that a call_srcu has happened for the srcu domain in a
>> scalable fashion, let's see... We could have a flag "call_srcu_used"
>> for each call_srcu domain. Whenever call_srcu is invoked, it would
>> load that flag. It sets it on first use.
>> 
>> The idea here is to only use that flag when srcu_barrier is performed
>> right before the srcu domain cleanup (it could become part of that
>> cleanup). Else, using it in all srcu_barrier() might be tricky, because
>> we may then need to add memory barriers or locking to the call_srcu
>> fast-path, which is an overhead we try to avoid.
>> 
>> However, if we only use that flag as part of the srcu domain cleanup,
>> it's already prohibited to invoke call_srcu concurrently with the
>> cleanup of the same domain, so I don't think we would need any
>> memory barriers in call_srcu.
> 
> About the last part of your email, it seems to that if after call_srcu has
> returned, if the module could be unloaded on some other CPU - then it would
> need to see the flag stored by the preceding call_srcu, so I believe there
> would be a memory barrier between the two opreations (call_srcu and module
> unload).

In order for the module unload not to race against module execution, it needs
to happen after the call_srcu in a way that is already ordered by other means,
else module unload races against the module code.

> 
> Also about doing the unconditional srcu_barrier, since a module could be
> unloaded at any time - don't all SRCU using modules need to invoke
> srcu_barrier() during their clean up anyway so we are incurring the barrier
> overhead anyway? Or, am I missing a design pattern here? It seems to me
> rcutorture module definitely calls srcu_barrier() before it is unloaded.

I think a valid approach which is even simpler might be: if a module statically
defines a SRCU domain, it should be expected to use it. So adding a srcu_barrier()
to its module going notifier should not hurt. The rare case where a module defines
a static SRCU domain *and* does not actually use it with call_srcu() does not
seem that usual, and not worth optimizing for.

Thoughts ?

Thanks,

Mathieu


> 
> thanks,
> 
> - Joel
> 
>> Thoughts ?
>> 
>> Thanks,
>> 
>> Mathieu
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
> > http://www.efficios.com

Joel Fernandes April 9, 2019, 4:18 p.m. UTC | #31

On Tue, Apr 09, 2019 at 11:56:03AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google [e-mail] wrote:
> 
> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> >> ----- On Apr 8, 2019, at 11:46 AM, paulmck [e-mail] wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> >> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
> >> >> 
> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
> >> >> >> 
> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
> >> >> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> > 
> >> >> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
> >> >> >> >> > wrote:
> >> >> >> >> > 
> >> >> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
> >> >> > 
> >> >> > So please let me try again.
> >> >> > 
> >> >> > If a given srcu_struct in a module never has call_srcu() invoked, there
> >> >> > is no need to invoke rcu_barrier() at any time, whether at module-unload
> >> >> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
> >> >> > for no good reason.
> >> >> 
> >> >> Not if we invoke srcu_barrier() for that specific domain. If
> >> >> call_srcu was never invoked for a srcu domain, I don't see why
> >> >> srcu_barrier() should be more expensive than a simple check that
> >> >> the domain does not have any srcu work queued.
> >> > 
> >> > But that simple check does involve a cache miss for each possible CPU (not
> >> > just each online CPU), so it is non-trivial, especially on large systems.
> >> > 
> >> >> > If a given srcu_struct in a module does have at least one call_srcu()
> >> >> > invoked, it is already that module's responsibility to make sure that
> >> >> > the code sticks around long enough for the callback to be invoked.
> >> >> 
> >> >> I understand that when users do explicit dynamic allocation/cleanup of
> >> >> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
> >> >> However, if they do static definition of srcu domains, it would be nice
> >> >> if we can handle the barriers under the hood.
> >> > 
> >> > All else being equal, of course.  But...
> >> > 
> >> >> > This means that correct SRCU users that invoke call_srcu() already
> >> >> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
> >> >> > reasonable probability, now get a WARN_ON() at module-unload time, with
> >> >> > the per-CPU state getting leaked.  Before this change, they would (also
> >> >> > with reasonable probability) instead get an instruction-fetch fault when
> >> >> > the SRCU callback was invoked after the completion of the module unload.
> >> >> > Furthermore, in all cases where they would previously have gotten the
> >> >> > instruction-fetch fault, they now get the WARN_ON(), like this:
> >> >> > 
> >> >> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
> >> >> >		return; /* Forgot srcu_barrier(), so just leak it! */
> >> >> > 
> >> >> > So this change already represents an improvement in usability.
> >> >> 
> >> >> Considering that we can do a srcu_barrier() for the specific domain,
> >> >> and that it should add no noticeable overhead if there is no queued
> >> >> callbacks, I don't see a good reason for leaving the srcu_barrier
> >> >> invocation to the user rather than implicitly doing it from the
> >> >> module going notifier.
> >> > 
> >> > Now, I could automatically add an indicator of whether or not a
> >> > call_srcu() had happened, but then again, that would either add a
> >> > call_srcu() scalability bottleneck or again require a scan of all possible
> >> > CPUs...  to figure out if it was necessary to scan all possible CPUs.
> >> > 
> >> > Or is scanning all possible CPUs down in the noise in this case?  Or
> >> > am I missing a trick that would reduce the overhead?
> >> 
> >> Module unloading implicitly does a synchronize_rcu (for RCU-sched), and
> >> a stop_machine. So I would be tempted to say that overhead of iteration
> >> over all CPUs might not matter that much considering the rest.
> >> 
> >> About notifying that a call_srcu has happened for the srcu domain in a
> >> scalable fashion, let's see... We could have a flag "call_srcu_used"
> >> for each call_srcu domain. Whenever call_srcu is invoked, it would
> >> load that flag. It sets it on first use.
> >> 
> >> The idea here is to only use that flag when srcu_barrier is performed
> >> right before the srcu domain cleanup (it could become part of that
> >> cleanup). Else, using it in all srcu_barrier() might be tricky, because
> >> we may then need to add memory barriers or locking to the call_srcu
> >> fast-path, which is an overhead we try to avoid.
> >> 
> >> However, if we only use that flag as part of the srcu domain cleanup,
> >> it's already prohibited to invoke call_srcu concurrently with the
> >> cleanup of the same domain, so I don't think we would need any
> >> memory barriers in call_srcu.
> > 
> > About the last part of your email, it seems to that if after call_srcu has
> > returned, if the module could be unloaded on some other CPU - then it would
> > need to see the flag stored by the preceding call_srcu, so I believe there
> > would be a memory barrier between the two opreations (call_srcu and module
> > unload).
> 
> In order for the module unload not to race against module execution, it needs
> to happen after the call_srcu in a way that is already ordered by other means,
> else module unload races against the module code.

Agreed.

> > Also about doing the unconditional srcu_barrier, since a module could be
> > unloaded at any time - don't all SRCU using modules need to invoke
> > srcu_barrier() during their clean up anyway so we are incurring the barrier
> > overhead anyway? Or, am I missing a design pattern here? It seems to me
> > rcutorture module definitely calls srcu_barrier() before it is unloaded.
> 
> I think a valid approach which is even simpler might be: if a module statically
> defines a SRCU domain, it should be expected to use it. So adding a srcu_barrier()
> to its module going notifier should not hurt. The rare case where a module defines
> a static SRCU domain *and* does not actually use it with call_srcu() does not
> seem that usual, and not worth optimizing for.

That sounds like a reasonable approach to me.

thanks,

 - Joel

Paul E. McKenney April 9, 2019, 4:40 p.m. UTC | #32

On Tue, Apr 09, 2019 at 11:56:03AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 9, 2019, at 11:40 AM, Joel Fernandes, Google [e-mail] wrote:
> 
> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> >> ----- On Apr 8, 2019, at 11:46 AM, paulmck [e-mail] wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> >> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
> >> >> 
> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
> >> >> >> 
> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
> >> >> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> > 
> >> >> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
> >> >> >> >> > wrote:
> >> >> >> >> > 
> >> >> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
> >> >> > 
> >> >> > So please let me try again.
> >> >> > 
> >> >> > If a given srcu_struct in a module never has call_srcu() invoked, there
> >> >> > is no need to invoke rcu_barrier() at any time, whether at module-unload
> >> >> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
> >> >> > for no good reason.
> >> >> 
> >> >> Not if we invoke srcu_barrier() for that specific domain. If
> >> >> call_srcu was never invoked for a srcu domain, I don't see why
> >> >> srcu_barrier() should be more expensive than a simple check that
> >> >> the domain does not have any srcu work queued.
> >> > 
> >> > But that simple check does involve a cache miss for each possible CPU (not
> >> > just each online CPU), so it is non-trivial, especially on large systems.
> >> > 
> >> >> > If a given srcu_struct in a module does have at least one call_srcu()
> >> >> > invoked, it is already that module's responsibility to make sure that
> >> >> > the code sticks around long enough for the callback to be invoked.
> >> >> 
> >> >> I understand that when users do explicit dynamic allocation/cleanup of
> >> >> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
> >> >> However, if they do static definition of srcu domains, it would be nice
> >> >> if we can handle the barriers under the hood.
> >> > 
> >> > All else being equal, of course.  But...
> >> > 
> >> >> > This means that correct SRCU users that invoke call_srcu() already
> >> >> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
> >> >> > reasonable probability, now get a WARN_ON() at module-unload time, with
> >> >> > the per-CPU state getting leaked.  Before this change, they would (also
> >> >> > with reasonable probability) instead get an instruction-fetch fault when
> >> >> > the SRCU callback was invoked after the completion of the module unload.
> >> >> > Furthermore, in all cases where they would previously have gotten the
> >> >> > instruction-fetch fault, they now get the WARN_ON(), like this:
> >> >> > 
> >> >> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
> >> >> >		return; /* Forgot srcu_barrier(), so just leak it! */
> >> >> > 
> >> >> > So this change already represents an improvement in usability.
> >> >> 
> >> >> Considering that we can do a srcu_barrier() for the specific domain,
> >> >> and that it should add no noticeable overhead if there is no queued
> >> >> callbacks, I don't see a good reason for leaving the srcu_barrier
> >> >> invocation to the user rather than implicitly doing it from the
> >> >> module going notifier.
> >> > 
> >> > Now, I could automatically add an indicator of whether or not a
> >> > call_srcu() had happened, but then again, that would either add a
> >> > call_srcu() scalability bottleneck or again require a scan of all possible
> >> > CPUs...  to figure out if it was necessary to scan all possible CPUs.
> >> > 
> >> > Or is scanning all possible CPUs down in the noise in this case?  Or
> >> > am I missing a trick that would reduce the overhead?
> >> 
> >> Module unloading implicitly does a synchronize_rcu (for RCU-sched), and
> >> a stop_machine. So I would be tempted to say that overhead of iteration
> >> over all CPUs might not matter that much considering the rest.
> >> 
> >> About notifying that a call_srcu has happened for the srcu domain in a
> >> scalable fashion, let's see... We could have a flag "call_srcu_used"
> >> for each call_srcu domain. Whenever call_srcu is invoked, it would
> >> load that flag. It sets it on first use.
> >> 
> >> The idea here is to only use that flag when srcu_barrier is performed
> >> right before the srcu domain cleanup (it could become part of that
> >> cleanup). Else, using it in all srcu_barrier() might be tricky, because
> >> we may then need to add memory barriers or locking to the call_srcu
> >> fast-path, which is an overhead we try to avoid.
> >> 
> >> However, if we only use that flag as part of the srcu domain cleanup,
> >> it's already prohibited to invoke call_srcu concurrently with the
> >> cleanup of the same domain, so I don't think we would need any
> >> memory barriers in call_srcu.
> > 
> > About the last part of your email, it seems to that if after call_srcu has
> > returned, if the module could be unloaded on some other CPU - then it would
> > need to see the flag stored by the preceding call_srcu, so I believe there
> > would be a memory barrier between the two opreations (call_srcu and module
> > unload).
> 
> In order for the module unload not to race against module execution, it needs
> to happen after the call_srcu in a way that is already ordered by other means,
> else module unload races against the module code.
> 
> > 
> > Also about doing the unconditional srcu_barrier, since a module could be
> > unloaded at any time - don't all SRCU using modules need to invoke
> > srcu_barrier() during their clean up anyway so we are incurring the barrier
> > overhead anyway? Or, am I missing a design pattern here? It seems to me
> > rcutorture module definitely calls srcu_barrier() before it is unloaded.
> 
> I think a valid approach which is even simpler might be: if a module statically
> defines a SRCU domain, it should be expected to use it. So adding a srcu_barrier()
> to its module going notifier should not hurt. The rare case where a module defines
> a static SRCU domain *and* does not actually use it with call_srcu() does not
> seem that usual, and not worth optimizing for.
> 
> Thoughts ?

Most SRCU users use only synchronize_srcu(), and don't ever use
call_srcu().  Which is not too surprising given that call_srcu() showed
up late in the game.

But something still bothers me about this, and I am not yet sure
what.  One thing that seems to reduce anxiety somewhat is doing the
srcu_barrier() on all calls to cleanup_srcu_struct() rather than just
those invoked from the modules infrastructure, but I don't see why at
the moment.

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> > thanks,
> > 
> > - Joel
> > 
> >> Thoughts ?
> >> 
> >> Thanks,
> >> 
> >> Mathieu
> >> 
> >> --
> >> Mathieu Desnoyers
> >> EfficiOS Inc.
> > > http://www.efficios.com
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
>

Mathieu Desnoyers April 9, 2019, 4:45 p.m. UTC | #33

----- On Apr 9, 2019, at 12:40 PM, paulmck [e-mail] 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 [e-mail]
>> wrote:
>> 
>> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
>> >> ----- On Apr 8, 2019, at 11:46 AM, paulmck [e-mail] wrote:
>> >> 
>> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
>> >> >> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
>> >> >> 
>> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
>> >> >> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
>> >> >> >> 
>> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
>> >> >> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
>> >> >> >> >> > 
>> >> >> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
>> >> >> >> >> > wrote:
>> >> >> >> >> > 
>> >> >> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
>> >> >> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
>> >> >> > 
>> >> >> > So please let me try again.
>> >> >> > 
>> >> >> > If a given srcu_struct in a module never has call_srcu() invoked, there
>> >> >> > is no need to invoke rcu_barrier() at any time, whether at module-unload
>> >> >> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
>> >> >> > for no good reason.
>> >> >> 
>> >> >> Not if we invoke srcu_barrier() for that specific domain. If
>> >> >> call_srcu was never invoked for a srcu domain, I don't see why
>> >> >> srcu_barrier() should be more expensive than a simple check that
>> >> >> the domain does not have any srcu work queued.
>> >> > 
>> >> > But that simple check does involve a cache miss for each possible CPU (not
>> >> > just each online CPU), so it is non-trivial, especially on large systems.
>> >> > 
>> >> >> > If a given srcu_struct in a module does have at least one call_srcu()
>> >> >> > invoked, it is already that module's responsibility to make sure that
>> >> >> > the code sticks around long enough for the callback to be invoked.
>> >> >> 
>> >> >> I understand that when users do explicit dynamic allocation/cleanup of
>> >> >> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
>> >> >> However, if they do static definition of srcu domains, it would be nice
>> >> >> if we can handle the barriers under the hood.
>> >> > 
>> >> > All else being equal, of course.  But...
>> >> > 
>> >> >> > This means that correct SRCU users that invoke call_srcu() already
>> >> >> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
>> >> >> > reasonable probability, now get a WARN_ON() at module-unload time, with
>> >> >> > the per-CPU state getting leaked.  Before this change, they would (also
>> >> >> > with reasonable probability) instead get an instruction-fetch fault when
>> >> >> > the SRCU callback was invoked after the completion of the module unload.
>> >> >> > Furthermore, in all cases where they would previously have gotten the
>> >> >> > instruction-fetch fault, they now get the WARN_ON(), like this:
>> >> >> > 
>> >> >> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
>> >> >> >		return; /* Forgot srcu_barrier(), so just leak it! */
>> >> >> > 
>> >> >> > So this change already represents an improvement in usability.
>> >> >> 
>> >> >> Considering that we can do a srcu_barrier() for the specific domain,
>> >> >> and that it should add no noticeable overhead if there is no queued
>> >> >> callbacks, I don't see a good reason for leaving the srcu_barrier
>> >> >> invocation to the user rather than implicitly doing it from the
>> >> >> module going notifier.
>> >> > 
>> >> > Now, I could automatically add an indicator of whether or not a
>> >> > call_srcu() had happened, but then again, that would either add a
>> >> > call_srcu() scalability bottleneck or again require a scan of all possible
>> >> > CPUs...  to figure out if it was necessary to scan all possible CPUs.
>> >> > 
>> >> > Or is scanning all possible CPUs down in the noise in this case?  Or
>> >> > am I missing a trick that would reduce the overhead?
>> >> 
>> >> Module unloading implicitly does a synchronize_rcu (for RCU-sched), and
>> >> a stop_machine. So I would be tempted to say that overhead of iteration
>> >> over all CPUs might not matter that much considering the rest.
>> >> 
>> >> About notifying that a call_srcu has happened for the srcu domain in a
>> >> scalable fashion, let's see... We could have a flag "call_srcu_used"
>> >> for each call_srcu domain. Whenever call_srcu is invoked, it would
>> >> load that flag. It sets it on first use.
>> >> 
>> >> The idea here is to only use that flag when srcu_barrier is performed
>> >> right before the srcu domain cleanup (it could become part of that
>> >> cleanup). Else, using it in all srcu_barrier() might be tricky, because
>> >> we may then need to add memory barriers or locking to the call_srcu
>> >> fast-path, which is an overhead we try to avoid.
>> >> 
>> >> However, if we only use that flag as part of the srcu domain cleanup,
>> >> it's already prohibited to invoke call_srcu concurrently with the
>> >> cleanup of the same domain, so I don't think we would need any
>> >> memory barriers in call_srcu.
>> > 
>> > About the last part of your email, it seems to that if after call_srcu has
>> > returned, if the module could be unloaded on some other CPU - then it would
>> > need to see the flag stored by the preceding call_srcu, so I believe there
>> > would be a memory barrier between the two opreations (call_srcu and module
>> > unload).
>> 
>> In order for the module unload not to race against module execution, it needs
>> to happen after the call_srcu in a way that is already ordered by other means,
>> else module unload races against the module code.
>> 
>> > 
>> > Also about doing the unconditional srcu_barrier, since a module could be
>> > unloaded at any time - don't all SRCU using modules need to invoke
>> > srcu_barrier() during their clean up anyway so we are incurring the barrier
>> > overhead anyway? Or, am I missing a design pattern here? It seems to me
>> > rcutorture module definitely calls srcu_barrier() before it is unloaded.
>> 
>> I think a valid approach which is even simpler might be: if a module statically
>> defines a SRCU domain, it should be expected to use it. So adding a
>> srcu_barrier()
>> to its module going notifier should not hurt. The rare case where a module
>> defines
>> a static SRCU domain *and* does not actually use it with call_srcu() does not
>> seem that usual, and not worth optimizing for.
>> 
>> Thoughts ?
> 
> Most SRCU users use only synchronize_srcu(), and don't ever use
> call_srcu().  Which is not too surprising given that call_srcu() showed
> up late in the game.
> 
> But something still bothers me about this, and I am not yet sure
> what.  One thing that seems to reduce anxiety somewhat is doing the
> srcu_barrier() on all calls to cleanup_srcu_struct() rather than just
> those invoked from the modules infrastructure, but I don't see why at
> the moment.

Indeed, providing similar guarantees for the dynamic allocation case
would be nice.

The one thing that is making me anxious here is use-cases where
users would decide to chain their call_srcu(). Then they would
need as many srcu_barrier() as chain hops. This would be a valid
reason for leaving invocation of srcu_barrier() to the user and
not hide it under the hood.

Thoughts ?

Thanks,

Mathieu


> 
>							Thanx, Paul
> 
>> Thanks,
>> 
>> Mathieu
>> 
>> 
>> > 
>> > thanks,
>> > 
>> > - Joel
>> > 
>> >> Thoughts ?
>> >> 
>> >> Thanks,
>> >> 
>> >> Mathieu
>> >> 
>> >> --
>> >> Mathieu Desnoyers
>> >> EfficiOS Inc.
>> > > http://www.efficios.com
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com

Paul E. McKenney April 9, 2019, 5:55 p.m. UTC | #34

On Tue, Apr 09, 2019 at 12:45:25PM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 9, 2019, at 12:40 PM, paulmck [e-mail] 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 [e-mail]
> >> wrote:
> >> 
> >> > On Mon, Apr 08, 2019 at 01:24:47PM -0400, Mathieu Desnoyers wrote:
> >> >> ----- On Apr 8, 2019, at 11:46 AM, paulmck [e-mail] wrote:
> >> >> 
> >> >> > On Mon, Apr 08, 2019 at 10:49:32AM -0400, Mathieu Desnoyers wrote:
> >> >> >> ----- On Apr 8, 2019, at 10:22 AM, paulmck [e-mail] wrote:
> >> >> >> 
> >> >> >> > On Mon, Apr 08, 2019 at 09:05:34AM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> ----- On Apr 7, 2019, at 10:27 PM, paulmck [e-mail] wrote:
> >> >> >> >> 
> >> >> >> >> > On Sun, Apr 07, 2019 at 09:07:18PM +0000, Joel Fernandes wrote:
> >> >> >> >> >> On Sun, Apr 07, 2019 at 04:41:36PM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> >> > 
> >> >> >> >> >> > ----- On Apr 7, 2019, at 3:32 PM, Joel Fernandes, Google [e-mail]
> >> >> >> >> >> > wrote:
> >> >> >> >> >> > 
> >> >> >> >> >> > > On Sun, Apr 07, 2019 at 03:26:16PM -0400, Mathieu Desnoyers wrote:
> >> >> >> >> >> > >> ----- On Apr 7, 2019, at 9:59 AM, paulmck [e-mail] 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 nothing to do with the order of events at module-unload time.
> >> >> >> > 
> >> >> >> > So please let me try again.
> >> >> >> > 
> >> >> >> > If a given srcu_struct in a module never has call_srcu() invoked, there
> >> >> >> > is no need to invoke rcu_barrier() at any time, whether at module-unload
> >> >> >> > time or not.  Adding rcu_barrier() in this case adds overhead and latency
> >> >> >> > for no good reason.
> >> >> >> 
> >> >> >> Not if we invoke srcu_barrier() for that specific domain. If
> >> >> >> call_srcu was never invoked for a srcu domain, I don't see why
> >> >> >> srcu_barrier() should be more expensive than a simple check that
> >> >> >> the domain does not have any srcu work queued.
> >> >> > 
> >> >> > But that simple check does involve a cache miss for each possible CPU (not
> >> >> > just each online CPU), so it is non-trivial, especially on large systems.
> >> >> > 
> >> >> >> > If a given srcu_struct in a module does have at least one call_srcu()
> >> >> >> > invoked, it is already that module's responsibility to make sure that
> >> >> >> > the code sticks around long enough for the callback to be invoked.
> >> >> >> 
> >> >> >> I understand that when users do explicit dynamic allocation/cleanup of
> >> >> >> srcu domains, they indeed need to take care of doing explicit srcu_barrier().
> >> >> >> However, if they do static definition of srcu domains, it would be nice
> >> >> >> if we can handle the barriers under the hood.
> >> >> > 
> >> >> > All else being equal, of course.  But...
> >> >> > 
> >> >> >> > This means that correct SRCU users that invoke call_srcu() already
> >> >> >> > have srcu_barrier() at module-unload time.  Incorrect SRCU users, with
> >> >> >> > reasonable probability, now get a WARN_ON() at module-unload time, with
> >> >> >> > the per-CPU state getting leaked.  Before this change, they would (also
> >> >> >> > with reasonable probability) instead get an instruction-fetch fault when
> >> >> >> > the SRCU callback was invoked after the completion of the module unload.
> >> >> >> > Furthermore, in all cases where they would previously have gotten the
> >> >> >> > instruction-fetch fault, they now get the WARN_ON(), like this:
> >> >> >> > 
> >> >> >> >	if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))
> >> >> >> >		return; /* Forgot srcu_barrier(), so just leak it! */
> >> >> >> > 
> >> >> >> > So this change already represents an improvement in usability.
> >> >> >> 
> >> >> >> Considering that we can do a srcu_barrier() for the specific domain,
> >> >> >> and that it should add no noticeable overhead if there is no queued
> >> >> >> callbacks, I don't see a good reason for leaving the srcu_barrier
> >> >> >> invocation to the user rather than implicitly doing it from the
> >> >> >> module going notifier.
> >> >> > 
> >> >> > Now, I could automatically add an indicator of whether or not a
> >> >> > call_srcu() had happened, but then again, that would either add a
> >> >> > call_srcu() scalability bottleneck or again require a scan of all possible
> >> >> > CPUs...  to figure out if it was necessary to scan all possible CPUs.
> >> >> > 
> >> >> > Or is scanning all possible CPUs down in the noise in this case?  Or
> >> >> > am I missing a trick that would reduce the overhead?
> >> >> 
> >> >> Module unloading implicitly does a synchronize_rcu (for RCU-sched), and
> >> >> a stop_machine. So I would be tempted to say that overhead of iteration
> >> >> over all CPUs might not matter that much considering the rest.
> >> >> 
> >> >> About notifying that a call_srcu has happened for the srcu domain in a
> >> >> scalable fashion, let's see... We could have a flag "call_srcu_used"
> >> >> for each call_srcu domain. Whenever call_srcu is invoked, it would
> >> >> load that flag. It sets it on first use.
> >> >> 
> >> >> The idea here is to only use that flag when srcu_barrier is performed
> >> >> right before the srcu domain cleanup (it could become part of that
> >> >> cleanup). Else, using it in all srcu_barrier() might be tricky, because
> >> >> we may then need to add memory barriers or locking to the call_srcu
> >> >> fast-path, which is an overhead we try to avoid.
> >> >> 
> >> >> However, if we only use that flag as part of the srcu domain cleanup,
> >> >> it's already prohibited to invoke call_srcu concurrently with the
> >> >> cleanup of the same domain, so I don't think we would need any
> >> >> memory barriers in call_srcu.
> >> > 
> >> > About the last part of your email, it seems to that if after call_srcu has
> >> > returned, if the module could be unloaded on some other CPU - then it would
> >> > need to see the flag stored by the preceding call_srcu, so I believe there
> >> > would be a memory barrier between the two opreations (call_srcu and module
> >> > unload).
> >> 
> >> In order for the module unload not to race against module execution, it needs
> >> to happen after the call_srcu in a way that is already ordered by other means,
> >> else module unload races against the module code.
> >> 
> >> > 
> >> > Also about doing the unconditional srcu_barrier, since a module could be
> >> > unloaded at any time - don't all SRCU using modules need to invoke
> >> > srcu_barrier() during their clean up anyway so we are incurring the barrier
> >> > overhead anyway? Or, am I missing a design pattern here? It seems to me
> >> > rcutorture module definitely calls srcu_barrier() before it is unloaded.
> >> 
> >> I think a valid approach which is even simpler might be: if a module statically
> >> defines a SRCU domain, it should be expected to use it. So adding a
> >> srcu_barrier()
> >> to its module going notifier should not hurt. The rare case where a module
> >> defines
> >> a static SRCU domain *and* does not actually use it with call_srcu() does not
> >> seem that usual, and not worth optimizing for.
> >> 
> >> Thoughts ?
> > 
> > Most SRCU users use only synchronize_srcu(), and don't ever use
> > call_srcu().  Which is not too surprising given that call_srcu() showed
> > up late in the game.
> > 
> > But something still bothers me about this, and I am not yet sure
> > what.  One thing that seems to reduce anxiety somewhat is doing the
> > srcu_barrier() on all calls to cleanup_srcu_struct() rather than just
> > those invoked from the modules infrastructure, but I don't see why at
> > the moment.
> 
> Indeed, providing similar guarantees for the dynamic allocation case
> would be nice.
> 
> The one thing that is making me anxious here is use-cases where
> users would decide to chain their call_srcu(). Then they would
> need as many srcu_barrier() as chain hops. This would be a valid
> reason for leaving invocation of srcu_barrier() to the user and
> not hide it under the hood.
> 
> Thoughts ?

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.  ;-)

							Thanx, Paul

Mathieu Desnoyers April 9, 2019, 6:04 p.m. UTC | #35

----- On Apr 9, 2019, at 1:55 PM, paulmck [e-mail] 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

Paul E. McKenney April 9, 2019, 7:14 p.m. UTC | #36

On Tue, Apr 09, 2019 at 02:04:11PM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 9, 2019, at 1:55 PM, paulmck [e-mail] 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
>