Date: Tue, 24 Aug 2010 10:02:34 -0400 From: Stephan Uphoff <ups@freebsd.org> To: Max Laier <max@laiers.net> Cc: freebsd-arch@freebsd.org Subject: Re: rmlock(9) two additions Message-ID: <4C73D0FA.5030102@freebsd.org> In-Reply-To: <201008240045.15998.max@laiers.net> References: <201008160515.21412.max@love2party.net> <4C7042BA.8000402@freebsd.org> <201008222105.47276.max@love2party.net> <201008240045.15998.max@laiers.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Max Laier wrote:
> On Sunday 22 August 2010 21:05:47 Max Laier wrote:
>
>> On Saturday 21 August 2010 23:18:50 Stephan Uphoff wrote:
>>
>>> Max Laier wrote:
>>> ...
>>> The rm_try_rlock() will never succeed when the lock was last locked as
>>> writer.
>>> Maybe add:
>>>
>>> void
>>> _rm_wunlock(struct rmlock *rm)
>>> {
>>> + rm->rm_noreadtoken = 0;
>>>
>>> mtx_unlock(&rm->rm_lock);
>>>
>>> }
>>>
>>> But then
>>>
>>> _rm_wlock(struct rmlock *rm)
>>>
>>> always needs to use IPIs - even when the lock was used last as a write
>>> lock.
>>>
>> I don't think this is a big problem - I can't see many use cases for
>> rmlocks where you'd routinely see repeated wlocks without rlocks between
>> them. However, I think there should be a release memory barrier
>> before/while clearing rm_noreadtoken, otherwise readers may not see the
>> data writes that are supposed to be protected by the lock?!?
>>
>>> ...
>>>
>> I believe either solution will work. #1 is a bit more in the spirit of the
>> rmlock - i.e. make the read case cheap and the write case expensive. I'm
>> just not sure about the lock semantics.
>>
>> I guess a
>>
>> atomic_store_rel_int(&rm->rm_noreadtoken, 0);
>>
>> should work.
>>
>
> thinking about this for a while makes me wonder: Are readers really guaranteed
> to see all the updates of a writer - even in the current version?
>
> Example:
>
> writer thread:
> rm_wlock(); // lock mtx, IPI, wait for reader drain
> modify_data();
> rm_wunlock(); // unlock mtx (this does a atomic_*_rel)
>
> reader thread #1:
> // failed to get the lock, spinning/waiting on mtx
> mtx_lock(); // this does a atomic_*_acq -> this CPU sees the new data
> rm->rm_noreadtoken = 0; // now new readers can enter quickly
> ...
>
> reader thread 2# (on a different CPU than reader #1):
> // enters rm_rlock() "after" rm_noreadtoken was reset -> no memory barrier
> // does this thread see the modifications?
>
> I realize this is a somewhat pathological case, but would it be possible in
> theory? Or is the compiler_memory_barrier() actually enough?
>
> Otherwise, I think we need an IPI on rm_wunlock() that does a atomic_*_acq on
> every CPU.
>
> Thoughts?
> Max
>
>
Yes - this is a problem that needs to be addressed.
Fortunately most platforms won't need to be as strict and I suggest per
platform parameters.
An alternative that was in my original design was to use a bitmap for
the rm_noreadtoken.
Each CPU would then have an associated bit that will only be cleared by
that cpu.
This would also allow targeted IPIs to only the token holders.
Stephan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4C73D0FA.5030102>