-----Original Message-----
>From: Boqun Feng [mailto:boqun.f...@gmail.com] 
>Sent: 2018年1月30日 14:41
>To: zhangheng (AC) <hen...@huawei.com>
>Cc: Paul E. McKenney <paul...@linux.vnet.ibm.com>; liangli...@huawei.com; 
>Guohanjun (Hanjun Guo) <guohan...@huawei.com>; Chenhaibo (Haibo, OS Lab) 
><hb.c...@huawei.com>; lihao.li...@gmail.com; linux-kernel@vger.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().

Agree.

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

Yes, you are right. Currently prcu hasn't given a solution for overflow thus it 
needs a 64-bit counter.
Giving a solution is not that difficult. I just didn't consider it when I use 
64-bit counter.
Since 64-bit counter isn't friendly for 32-bit system, I agree that 32-bit 
counter + overflow handler is necessary.
Thank you.

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

Reply via email to