On 12/12/2012 10:47 PM, Oleg Nesterov wrote: > On 12/11, Srivatsa S. Bhat wrote: >> >> IOW, the hotplug readers just increment/decrement their per-cpu refcounts >> when no writer is active. > > plus cli/sti ;)
Of course, forgot to mention it, again! :) > and increment/decrement are atomic. > > At first glance looks correct to me, but I'll try to read it carefully > later. > > A couple of minor nits, > >> +static DEFINE_PER_CPU(bool, writer_signal); > > Why it needs to be per-cpu? It can be global and __read_mostly to avoid > the false-sharing. OK, perhaps to put reader_percpu_refcnt/writer_signal > into a single cacheline... > Even I realized this (that we could use a global) after posting out the series.. But do you think that it would be better to retain the per-cpu variant itself, due to the cache effects? >> +void get_online_cpus_atomic(void) >> +{ >> + unsigned long flags; >> + >> + preempt_disable(); >> + >> + if (cpu_hotplug.active_writer == current) >> + return; >> + >> + local_irq_save(flags); > > Yes... this is still needed, we are going to increment reader_percpu_refcnt > unconditionally and this makes reader_nested_percpu() == T. > > But, > >> +void put_online_cpus_atomic(void) >> +{ >> + unsigned long flags; >> + >> + if (cpu_hotplug.active_writer == current) >> + goto out; >> + >> + local_irq_save(flags); >> + >> + /* >> + * We never allow heterogeneous nesting of readers. So it is trivial >> + * to find out the kind of reader we are, and undo the operation >> + * done by our corresponding get_online_cpus_atomic(). >> + */ >> + if (__this_cpu_read(reader_percpu_refcnt)) >> + __this_cpu_dec(reader_percpu_refcnt); >> + else >> + read_unlock(&hotplug_rwlock); >> + >> + local_irq_restore(flags); >> +out: >> + preempt_enable(); >> +} > > Do we really need local_irq_save/restore in put_ ? > Hmm.. good point! I don't think we need it. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/