Messages in this thread | | | Date | Thu, 18 Oct 2012 19:57:47 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/2] brw_mutex: big read-write mutex |
| |
On 10/18, Paul E. McKenney wrote: > > On Thu, Oct 18, 2012 at 06:24:09PM +0200, Oleg Nesterov wrote: > > > > I thought that you meant that without mb() brw_start_write() can > > race with brw_end_read() and hang forever. > > > > But probably you meant that we need the barriers to ensure that, > > say, if the reader does > > > > brw_start_read(); > > CONDITION = 1; > > brw_end_read(); > > > > then the writer must see CONDITION != 0 after brw_start_write() ? > > (or vice-versa) > > Yes, this is exactly my concern.
Oh, thanks at lot Paul (as always).
> > In this case we need the barrier, yes. Obviously brw_start_write() > > can return right after this_cpu_dec() and before wake_up_all(). > > > > 2/2 doesn't need this guarantee but I agree, this doesn't look > > sane in gerenal... > > Or name it something not containing "lock". And clearly document > the behavior and how it is to be used. ;-)
this would be insane, I guess ;)
So. Ignoring the possible optimization you mentioned before, brw_end_read() should do:
smp_mb(); this_cpu_dec();
wake_up_all();
And yes, we need the full mb(). wmb() is enough to ensure that the writer will see the memory modifications done by the reader. But we also need to ensure that any LOAD inside start_read/end_read can not be moved outside of the critical section.
But we should also ensure that "read" will see all modifications which were done under start_write/end_write. This means that brw_end_write() needs another synchronize_sched() before atomic_dec_and_test(), or brw_start_read() needs mb() in the fast-path.
Correct?
Ooooh. And I just noticed include/linux/percpu-rwsem.h which does something similar. Certainly it was not in my tree when I started this patch... percpu_down_write() doesn't allow multiple writers, but the main problem it uses msleep(1). It should not, I think.
But. It seems that percpu_up_write() is equally wrong? Doesn't it need synchronize_rcu() before "p->locked = false" ?
(add Mikulas)
Oleg.
| |