Hey,
Op 14-02-13 11:22, Arnd Bergmann schreef:
On Thursday 07 February 2013, Maarten Lankhorst wrote:
Hi Maarten,
I cannot help a lot on this patch set, but there are a few things that I noticed when reading it.
Functions:
mutex_reserve_lock, and mutex_reserve_lock_interruptible: Lock a buffer with a reservation_id set. reservation_id must not be set to 0, since this is a special value that means no reservation_id.
I think the entire description should go into a file in the Documentation directory, to make it easier to find without looking up the git history.
For the purpose of documenting this, it feels a little strange to talk about "buffers" here. Obviously this is what you are using the locks for, but it sounds like that is not the only possible use case.
It is the idea it will end up in Documentation, however I had a hard time even getting people to review the code, so I found it easier to keep code and documentation in sync by keeping it into the commit log when I was amending things.
But yes it's the use case I use it for. The generic use case would be if you had a number of equal locks you want to take in an arbitrary order without deadlocking or a lock protecting all those locks*.
*) In the eyes of lockdep you still take one of those locks, and nest all those locks you take into that lock.
These functions will return -EDEADLK instead of -EAGAIN if reservation_id is the same as the reservation_id that's attempted to lock the mutex with, since in that case you presumably attempted to lock the same lock twice.
Since the user always has to check the return value, would it be possible to provide only the interruptible kind of this function but not the non-interruptible one? In general, interruptible locks are obviously harder to use, but they are much user friendlier when something goes wrong.
I agree that in general you want to use the interruptible versions as much as possible, but there are some cases in ttm where it is desirable to lock non-interruptibly.
mutex_reserve_lock_slow and mutex_reserve_lock_intr_slow: Similar to mutex_reserve_lock, except it won't backoff with -EAGAIN. This is useful when mutex_reserve_lock failed with -EAGAIN, and you unreserved all buffers so no deadlock can occur.
Are these meant to be used a lot? If not, maybe prefix them with __mutex_ instead of mutex_.
It is a common path in case of contestion. The example lock_execbuf from the commit log used it. When you use the mutex_reserve_lock calls, you'll have to add calls to the *_slow variants too when those return -EAGAIN.
diff --git a/include/linux/mutex.h b/include/linux/mutex.h index 9121595..602c247 100644 --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -62,6 +62,11 @@ struct mutex { #endif }; +struct ticket_mutex {
- struct mutex base;
- atomic_long_t reservation_id;
+};
Have you considered changing the meaning of the "count" member of the mutex in the case where a ticket mutex is used? That would let you use an unmodified structure.
I have considered it, but I never found a good way to make that happen. mutex_lock and mutex_unlock are currently still used when only a single buffer needs to be locked.
Thanks for the review!
~Maarten