| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 30 Jan 2018 14:40:57 +0800
From: Boqun Feng <boqun.feng@...il.com>
To: "zhangheng (AC)" <heng.z@...wei.com>
Cc: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
"lianglihao@...wei.com" <lianglihao@...wei.com>,
"Guohanjun (Hanjun Guo)" <guohanjun@...wei.com>,
"Chenhaibo (Haibo, OS Lab)" <hb.chen@...wei.com>,
"lihao.liang@...il.com" <lihao.liang@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 01/16] prcu: Add PRCU implementation
On Tue, Jan 30, 2018 at 05:34:03AM +0000, zhangheng (AC) wrote:
[...]
> >> > +static void prcu_handler(void *info) {
> >> > + struct prcu_local_struct *local;
> >> > +
> >> > + local = this_cpu_ptr(&prcu_local);
> >> > + if (!local->locked)
> >
> >And I think a smp_mb() is needed here, because in the following case:
> >
> > CPU 0 CPU 1
> > ================== ==========================
> > {X is initially 0}
> >
> > WRITE_ONCE(X, 1);
> >
> > prcu_read_unlock(void):
> > if (locked) {
> > synchronize_prcu(void):
> > ...
> > <send IPI to CPU 0>
> > local->locked--;
> > # switch to IPI
> > WRITE_ONCE(local->version,....)
> > <read CPU 0 version to be latest>
> > <return>
> >
> > r1 = READ_ONCE(X);
> >
> >r1 could be 0, which breaks RCU guarantees.
> >
>
> Thank you.
> As I know,
> it guarantees that the interrupt to be handled after all write instructions issued before have complete in x86 arch.
> So the smp_mb is meaningless in x86 arch.
Sure. x86 is TSO, and we are talking about reordering of two stores
here, and that can not happen on TSO.
> But I am not sure whether other archs guarantee this feature. If not, we do need a smp_mb here.
>
I think most of the weak memory model don't have this gaurantee, so you
need a smp_mb() or use smp_store_release().
> >> > + WRITE_ONCE(local->version, atomic64_read(&prcu->global_version));
> >> > +}
> >> > +
> >> > +void synchronize_prcu(void)
> >> > +{
> >> > + int cpu;
> >> > + cpumask_t cpus;
> >> > + unsigned long long version;
> >> > + struct prcu_local_struct *local;
> >> > +
> >> > + version = atomic64_add_return(1, &prcu->global_version);
> >> > + mutex_lock(&prcu->mtx);
> >> > +
> >> > + local = get_cpu_ptr(&prcu_local);
> >> > + local->version = version;
> >> > + put_cpu_ptr(&prcu_local);
> >> > +
> >> > + cpumask_clear(&cpus);
> >> > + for_each_possible_cpu(cpu) {
> >> > + local = per_cpu_ptr(&prcu_local, cpu);
> >> > + if (!READ_ONCE(local->online))
> >> > + continue;
> >> > + if (READ_ONCE(local->version) < version) {
> >>
> >> On 32-bit systems, given that ->version is long long, you might see
> >> load tearing. And on some 32-bit systems, the cmpxchg() in
> >> prcu_hander() might not build.
> >>
> >
> >/me curious about why an atomic64_t is used here for global version. I think maybe 32bit global version still suffices.
> >
> >Regards,
> >Boqun
>
> Because the synchronization latency is low, it can have higher gp frequency.
> It seems that 32bit can only correctly work for several years if there are 20+ gps per second.
>
Because PRCU doesn't handle gp number overflow? May I ask why this is
difficult? Currently RCU could tolerate counter wrap for grace period:
https://lwn.net/Articles/652677/ (Details in "Parallelism facts of life")
Is there any subtle difference I'm missing?
Regards,
Boqun
> >
> >> Or is the idea that only prcu_handler() updates ->version? But in
> >> that case, you wouldn't need the READ_ONCE() above. What am I missing here?
> >>
> >> > + smp_call_function_single(cpu, prcu_handler, NULL, 0);
> >> > + cpumask_set_cpu(cpu, &cpus);
> >> > + }
> >> > + }
> >> > +
> >> > + for_each_cpu(cpu, &cpus) {
> >> > + local = per_cpu_ptr(&prcu_local, cpu);
> >> > + while (READ_ONCE(local->version) < version)
> >>
> >> This ->version read can also tear on some 32-bit systems, and this one
> >> most definitely can race with the prcu_handler() above. Does the
> >> algorithm operate correctly in that case? (It doesn't look that way
> >> to me, but I might be missing something.) Or are 32-bit systems excluded?
> >>
> >> > + cpu_relax();
> >> > + }
> >>
> >> I might be missing something, but I believe we need a memory barrier
> >> here on non-TSO systems. Without that, couldn't we miss a preemption?
> >>
> >> > +
> >> > + if (atomic_read(&prcu->active_ctr))
> >> > + wait_event(prcu->wait_q, !atomic_read(&prcu->active_ctr));
> >> > +
> >> > + mutex_unlock(&prcu->mtx);
> >> > +}
> >> > +EXPORT_SYMBOL(synchronize_prcu);
> >> > +
> >> > +void prcu_note_context_switch(void) {
> >> > + struct prcu_local_struct *local;
> >> > +
> >> > + local = get_cpu_ptr(&prcu_local);
> >> > + if (local->locked) {
> >> > + atomic_add(local->locked, &prcu->active_ctr);
> >> > + local->locked = 0;
> >> > + }
> >> > + local->online = 0;
> >> > + prcu_report(local);
> >> > + put_cpu_ptr(&prcu_local);
> >> > +}
> >> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c index
> >> > 326d4f88..a308581b 100644
> >> > --- a/kernel/sched/core.c
> >> > +++ b/kernel/sched/core.c
> >> > @@ -15,6 +15,7 @@
> >> > #include <linux/init_task.h>
> >> > #include <linux/context_tracking.h> #include
> >> > <linux/rcupdate_wait.h>
> >> > +#include <linux/prcu.h>
> >> >
> >> > #include <linux/blkdev.h>
> >> > #include <linux/kprobes.h>
> >> > @@ -3383,6 +3384,7 @@ static void __sched notrace __schedule(bool
> >> > preempt)
> >> >
> >> > local_irq_disable();
> >> > rcu_note_context_switch(preempt);
> >> > + prcu_note_context_switch();
> >> >
> >> > /*
> >> > * Make sure that signal_pending_state()->signal_pending() below
> >> > --
> >> > 2.14.1.729.g59c0ea183
> >> >
> >>
> >
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists