Le 12/09/2016 à 10:45 AM, Savolainen, Petri (Nokia - FI/Espoo) a écrit : > >> -----Original Message----- >> From: Nicolas Morey-Chaisemartin [mailto:nmo...@kalray.eu] >> Sent: Thursday, December 08, 2016 5:30 PM >> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- >> labs.com>; LNG ODP Mailman List <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] odp_rwlock_read_trylock >> >> >> >> Le 12/08/2016 à 03:50 PM, Savolainen, Petri (Nokia - FI/Espoo) a écrit : >>>> -----Original Message----- >>>> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of >>>> Nicolas Morey-Chaisemartin >>>> Sent: Wednesday, December 07, 2016 10:12 AM >>>> To: LNG ODP Mailman List <lng-odp@lists.linaro.org> >>>> Subject: [lng-odp] odp_rwlock_read_trylock >>>> >>>> HI, >>>> >>>> >>>> While looking into the test/code, I noticed something weird in the >>>> implementation of the rwlock read trylock on monarch. >>>> >>>> int odp_rwlock_read_trylock(odp_rwlock_t *rwlock) >>>> { >>>> uint32_t zero = 0; >>>> >>>> return odp_atomic_cas_acq_u32(&rwlock->cnt, &zero, (uint32_t)1); >>>> } >>>> >>>> >>>> This mean the trylock only succeed if no one was using the lock. But it >>>> will fail if someone was owning it *even* if it is a reader. >>>> Is this something expected? If yes, it should probably be detailed in >> the >>>> API. >>> /** >>> * Try to acquire read permission to a reader/writer lock. >>> * >>> * @param rwlock Pointer to a reader/writer lock >>> * >>> * @retval 0 Lock was not available for read access >>> * @retval !0 Read access to lock acquired >>> */ >>> int odp_rwlock_read_trylock(odp_rwlock_t *rwlock); >>> >>> >>> The spec just says that "read access" was not available. Implementation >> decides when it's available and application just needs to re-try later on. >> So, this implementation is OK by the spec. Supporting multiple readers >> would improve parallelism, but it may not be optimal for performance if >> the lock is free in the common case. >>> >>>> The lock implementation I wrote allows to get the lock if a reader >> already >>>> owns it. And causes the testsuite to deadlock: >>>> >>>> odp_rwlock_init(rw_lock); >>>> /* CU_ASSERT(odp_rwlock_is_locked(rw_lock) == 0); */ >>>> >>>> odp_rwlock_read_lock(rw_lock); => Lock is owned in read >>>> >>>> rc = odp_rwlock_read_trylock(rw_lock); => Locked is owned a second >>>> time in read (rc = 1) >>> You cannot do this on a single thread (lock the same lock twice). You >> must use recursive locks if the thread holding a lock will lock it again. >> The issue is odp_rwlock_read_trylock fails here, but odp_rwlock_read_lock >> would succeed. >> Usually rwlock are recursive on the read side as it doesn't really matter >> to the lock if the same thread owns it N times, or N threads own it once >> each. >> IMHO, in a non-concurrent setup, read_trylock should always succeed if >> read_lock would succeed. >> (I say non concurrent here to not acount for the case of contention on the >> lock where many retries could be needed to get the lock). > > We have recursive version of the lock (odp_rwlock_recursive_read_lock(), etc) > and application should use that if the same thread will call lock() twice (or > lock + try_lock).
Yes, but the test is not doing that. I think we have to distinguished the proper usage and what works. The proper usage if you want to do lock + lock or lock + trylock is to use a recursive rw lock. But if you do that on a non recursive one, what is supposed to happen? Right now lock + lock works and lock + trylock fails which doesn't feel natural. > >> What bothers me is this: >> >> A thread that wants write access will have to wait until >> * there are no threads that want read access. >> >> This clearly states that a writer cannot get the write lock if a reader >> *wants* it. >> I think >> >> A thread that wants write access will have to wait until >> * there are no threads that own read permission on the lock. >> >> >> would relax this constraint and allow for write-priority rwlock. > > That's OK wording. > > -Petri > > I'll send a patch for that. Should it go in all branches (including monarch) ? Nicolas
