Messages in this thread | | | Date | Fri, 19 Oct 2012 12:28:38 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH 1/2] brw_mutex: big read-write mutex |
| |
On Thu, Oct 18, 2012 at 07:57:47PM +0200, Oleg Nesterov wrote: > 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).
Glad it helped. ;-)
> > > 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 ;)
Well, I suppose you could call it a "phase" : brw_start_phase_1() and so on.
> 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?
Good point, I missed the need for synchronize_sched() to avoid readers sleeping through the next write cycle due to racing with an exiting writer. But yes, this sounds 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)
Mikulas said something about doing an updated patch, so I figured I would look at his next version.
Thanx, Paul
| |