On Wed, Dec 18, 2024 at 08:33:09PM +0100, Magnus Lindholm wrote:
> Hi again,
>
> So, various versions of GCC can trigger/untrigger the RCU bugs that
> I've been hitting. This may of course still be due to GCC miscompiling
> some code on alpha but that said I've taken another look at the code
> involved when this is triggered. From what I've seen so far, this
> happens when kernel threads are used in the code to access structured
> data stored on the stack (structs). The quote below is from a comment
> in the kernel code (include/linux/percpu-defs.h)
>
> "s390 and alpha modules require percpu variables to be defined as weak
> to force the compiler to generate GOT based external references for
> them. This is necessary because percpu sections will be located
> outside of the usually addressable area. This definition puts the
> following two extra restrictions when defining percpu variables.
>
> 1. The symbol must be globally unique, even the static ones.
>
> 2. Static percpu variables cannot be defined inside a function."
>
>
> Taking these notes into account I've made some minor modifications to
> the code (briefly described below). The modifications involve
> declaring some structs previously placed on stack as
> DEFINE_PER_CPU_SHARED_ALIGNED. This is already done for some strucs in
> rcu/smp code. Making these modifications fixes the problem and I can
> build the kernel with GCC versions that previously triggered the bug.
> The same approach fixed both the network interface-rename bug as well
> as the scsi module unload bug.
>
> in kernel/smp/smp.c
> -------------------
> #if defined(ARCH_NEEDS_WEAK_PER_CPU)
> smp_call_function_single(...) use csd = this_cpu_ptr(&csd_data)
> regardless if its called with wait = 0 or 1. Make sure to declare
> csd_data as "DEFINE_PER_CPU_SHARED_ALIGNED"
> #endif
If I understand your proposed changes correctly, this could increase
the amount of time CPUs spend waiting for for the csd_data to become free.
This would be a very unwelcome development in some quarters.
> in kernel/rcu/tree.c
> --------------------
> #use rew_data instead of stack allocated struct
> static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_exp_work, rew_data);
I am not finding this one.
> in kernel/rcu/tree_exp.h
> ------------------------
> #if defined(ARCH_NEEDS_WEAK_PER_CPU)
> void synchronize_rcu_expedited(void)
>
> in stead of:
> struct rcu_exp_work rew;
>
> do:
> #if defined(ARCH_NEEDS_WEAK_PER_CPU)
> struct rcu_exp_work *rewp;
> rewp=this_cpu_ptr(&rew_data);
>
> rew_data is declared "DEFINE_PER_CPU_SHARED_ALIGNED"
>
> #endif
OK, I *can* find this one, thank you. At least assuming you mean the
local variable "rew" defined in synchronize_rcu_expedited().
I don't see how this relates to the per-CPU variable restrictions called
out above relates here. We do not have any sort of per-CPU variable here,
but instead a simple structure allocated on the stack. And a relatively
small structure at that.
In addition, making this be a per-CPU variable will break if a given
CPU invokes a second synchronize_rcu_expedited() before its first call
to synchronize_rcu_expedited() returns. You will end up with the second
call clobbering that per-CPU variable, which will likely fatally confuse
the workqueue when attempting to manage the resulting workqueue handlers.
You might need heavy load to make this happen, but I don't see anything
preventing it from happening.
So what am I missing here?
Thanx, Paul
