Re: [Linaro-mm-sig] [PATCH 2/3] mutex: add support for reservation style locks

[Arnd Bergmann <arnd@arndb.de>]

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.

>   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.

> 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_.

> 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.

	Arnd