Messages in this thread | | | Date | Mon, 16 Sep 2013 20:37:53 -0400 | From | Peter Hurley <> | Subject | Re: [PATCH] rwsem: add rwsem_is_contended |
| |
On 09/16/2013 08:29 PM, David Daney wrote: > On 09/16/2013 05:05 PM, Josef Bacik wrote: >> On Mon, Sep 16, 2013 at 04:05:47PM -0700, Andrew Morton wrote: >>> On Fri, 30 Aug 2013 10:14:01 -0400 Josef Bacik <jbacik@fusionio.com> wrote: >>> >>>> Btrfs uses an rwsem to control access to its extent tree. Threads will hold a >>>> read lock on this rwsem while they scan the extent tree, and if need_resched() >>>> they will drop the lock and schedule. The transaction commit needs to take a >>>> write lock for this rwsem for a very short period to switch out the commit >>>> roots. If there are a lot of threads doing this caching operation we can starve >>>> out the committers which slows everybody out. To address this we want to add >>>> this functionality to see if our rwsem has anybody waiting to take a write lock >>>> so we can drop it and schedule for a bit to allow the commit to continue. >>>> Thanks, >>>> >>> >>> This sounds rather nasty and hacky. Rather then working around a >>> locking shortcoming in a caller it would be better to fix/enhance the >>> core locking code. What would such a change need to do? >>> >>> Presently rwsem waiters are fifo-queued, are they not? So the commit >>> thread will eventually get that lock. Apparently that's not working >>> adequately for you but I don't fully understand what it is about these >>> dynamics which is causing observable problems. >>> >> >> So the problem is not that its normal lock starvation, it's more our particular >> use case that is causing the starvation. We can have lots of people holding >> readers and simply never give them up for long periods of time, which is why we >> need this is_contended helper so we know to drop things and let the committer >> through. Thanks, > > You could easily achieve the same thing by putting an "is_contending" flag in parallel with the rwsem and testing that:
Which adds a bunch more bus-locked operations to contended over, when a unlocked if (list_empty()) is sufficient.
Regards, Peter Hurley
| |