Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1423305 > unrolled thread

initialize a mutex into locked state?

Back to article view | Back to linux.kernel

Contents

  initialize a mutex into locked state? Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 20:30 +0200
    Re: initialize a mutex into locked state? Peter Zijlstra <peterz@infradead.org> - 2016-06-17 10:30 +0200
      Re: initialize a mutex into locked state? Oleg Drokin <green@linuxhacker.ru> - 2016-06-17 16:20 +0200
        Re: initialize a mutex into locked state? Peter Zijlstra <peterz@infradead.org> - 2016-06-17 16:20 +0200
          Re: initialize a mutex into locked state? Oleg Drokin <green@linuxhacker.ru> - 2016-06-17 16:30 +0200
            Re: initialize a mutex into locked state? Peter Zijlstra <peterz@infradead.org> - 2016-06-17 16:40 +0200
              Re: initialize a mutex into locked state? Oleg Drokin <green@linuxhacker.ru> - 2016-06-17 16:50 +0200
                Re: initialize a mutex into locked state? Jeff Layton <jlayton@poochiereds.net> - 2016-06-17 16:50 +0200
                  Re: initialize a mutex into locked state? Oleg Drokin <green@linuxhacker.ru> - 2016-06-17 17:00 +0200
                    Re: initialize a mutex into locked state? Arjan van de Ven <arjan@linux.intel.com> - 2016-06-17 17:00 +0200
                Re: initialize a mutex into locked state? Peter Zijlstra <peterz@infradead.org> - 2016-06-17 17:10 +0200
            Re: initialize a mutex into locked state? Oleg Drokin <green@linuxhacker.ru> - 2016-06-17 16:50 +0200

#1423305 — initialize a mutex into locked state?

Hello!

  To my surprise I found out that it's not possible to initialise a mutex into
  a locked state.
  I discussed it with Arjan and apparently there's no fundamental reason
  not to allow this.

  A typical use would be when you have a structure that you need to init and then
  add to some list (under a spinlock), but with a possibility of a racing thread
  adding something there that you might want to reuse and not init your own copy
  of it to save cpu cycles. Yet you still want this structure to remain
  internally locked for other purposes before letting other threads do stuff
  with it (hence the mutex initialized to a locked state).

  There's just a case in nfsd that I met that would benefit from it that
  causes me to move mutex init + lock into the general "do always" area
  instead of doing it only when really necessary along with other init code
  (see the patch at http://marc.info/?l=linux-nfs&m=146596158830108&w=2 if
  interested).

  So I wonder if such a functionality should be considered?

Something like this (only compile-tested):

[PATCH] mutex: Add mutex_init_locked()

This is useful when you want to init it to this state under a spinlock.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
drivers/base/bus.c                             |  2 +-
drivers/base/class.c                           |  2 +-
drivers/gpu/drm/nouveau/nvkm/core/subdev.c     |  2 +-
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/base.c |  2 +-
include/linux/mutex-debug.h                    | 11 ++++++++++-
include/linux/mutex.h                          | 22 +++++++++++++++++++---
include/linux/ww_mutex.h                       |  2 +-
kernel/locking/mutex.c                         | 10 +++++++---
net/netfilter/ipvs/ip_vs_sync.c                |  2 +-
9 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/include/linux/mutex-debug.h b/include/linux/mutex-debug.h
index 4ac8b19..ee54a3d 100644
--- a/include/linux/mutex-debug.h
+++ b/include/linux/mutex-debug.h
@@ -16,9 +16,18 @@
do {									\
	static struct lock_class_key __key;				\
									\
-	__mutex_init((mutex), #mutex, &__key);				\
+	__mutex_init((mutex), #mutex, &__key, 0);			\
} while (0)

+#define mutex_init_locked(mutex)					\
+do {									\
+	static struct lock_class_key __key;				\
+									\
+	__mutex_init((mutex), #mutex, &__key, 1);			\
+} while (0)
+
+
+
extern void mutex_destroy(struct mutex *lock);

#endif
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2cb7531..43faca4 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -88,14 +88,30 @@ struct mutex_waiter {
 *
 * Initialize the mutex to unlocked state.
 *
- * It is not allowed to initialize an already locked mutex.
+ * It is not allowed to re-initialize an already locked mutex.
 */
# define mutex_init(mutex) \
do {							\
	static struct lock_class_key __key;		\
							\
-	__mutex_init((mutex), #mutex, &__key);		\
+	__mutex_init((mutex), #mutex, &__key, 0);	\
} while (0)
+
+/**
+ * mutex_init_locked - initialize the mutex
+ * @mutex: the mutex to be initialized
+ *
+ * Initialize the mutex to locked state.
+ *
+ * It is not allowed to re-initialize an already locked mutex.
+ */
+# define mutex_init_locked(mutex) \
+do {							\
+	static struct lock_class_key __key;		\
+							\
+	__mutex_init((mutex), #mutex, &__key, 1);	\
+} while (0)
+
static inline void mutex_destroy(struct mutex *lock) {}
#endif

@@ -117,7 +133,7 @@ static inline void mutex_destroy(struct mutex *lock) {}
	struct mutex mutexname = __MUTEX_INITIALIZER(mutexname)

extern void __mutex_init(struct mutex *lock, const char *name,
-			 struct lock_class_key *key);
+			 struct lock_class_key *key, int locked);

/**
 * mutex_is_locked - is the mutex locked
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index e364b42..a9cc497 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -47,12 +47,16 @@
#endif

void
-__mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
+__mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key,
+	     int locked)
{
-	atomic_set(&lock->count, 1);
+	atomic_set(&lock->count, !lock);
	spin_lock_init(&lock->wait_lock);
	INIT_LIST_HEAD(&lock->wait_list);
-	mutex_clear_owner(lock);
+	if (locked)
+		mutex_set_owner(lock);
+	else
+		mutex_clear_owner(lock);
#ifdef CONFIG_MUTEX_SPIN_ON_OWNER
	osq_lock_init(&lock->osq);
#endif
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 6470eb8..6df6bb3 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -930,7 +930,7 @@ int bus_register(struct bus_type *bus)
	}

	INIT_LIST_HEAD(&priv->interfaces);
-	__mutex_init(&priv->mutex, "subsys mutex", key);
+	__mutex_init(&priv->mutex, "subsys mutex", key, 0);
	klist_init(&priv->klist_devices, klist_devices_get, klist_devices_put);
	klist_init(&priv->klist_drivers, NULL, NULL);

diff --git a/drivers/base/class.c b/drivers/base/class.c
index 71059e3..a7e8031 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -176,7 +176,7 @@ int __class_register(struct class *cls, struct lock_class_key *key)
	klist_init(&cp->klist_devices, klist_class_dev_get, klist_class_dev_put);
	INIT_LIST_HEAD(&cp->interfaces);
	kset_init(&cp->glue_dirs);
-	__mutex_init(&cp->mutex, "subsys mutex", key);
+	__mutex_init(&cp->mutex, "subsys mutex", key, 0);
	error = kobject_set_name(&cp->subsys.kobj, "%s", cls->name);
	if (error) {
		kfree(cp);
diff --git a/drivers/gpu/drm/nouveau/nvkm/core/subdev.c b/drivers/gpu/drm/nouveau/nvkm/core/subdev.c
index b185578..7b0b022 100644
--- a/drivers/gpu/drm/nouveau/nvkm/core/subdev.c
+++ b/drivers/gpu/drm/nouveau/nvkm/core/subdev.c
@@ -198,6 +198,6 @@ nvkm_subdev_ctor(const struct nvkm_subdev_func *func,
	subdev->device = device;
	subdev->index = index;

-	__mutex_init(&subdev->mutex, name, &nvkm_subdev_lock_class[index]);
+	__mutex_init(&subdev->mutex, name, &nvkm_subdev_lock_class[index], 0);
	subdev->debug = nvkm_dbgopt(device->dbgopt, name);
}
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/base.c
index 5df9669..57fa64e 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/base.c
@@ -372,7 +372,7 @@ nvkm_vm_create(struct nvkm_mmu *mmu, u64 offset, u64 length, u64 mm_offset,
	if (!vm)
		return -ENOMEM;

-	__mutex_init(&vm->mutex, "&vm->mutex", key ? key : &_key);
+	__mutex_init(&vm->mutex, "&vm->mutex", key ? key : &_key, 0);
	INIT_LIST_HEAD(&vm->pgd_list);
	vm->mmu = mmu;
	kref_init(&vm->refcount);
diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 760399a..22bc051 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -85,7 +85,7 @@ struct ww_mutex {
static inline void ww_mutex_init(struct ww_mutex *lock,
				 struct ww_class *ww_class)
{
-	__mutex_init(&lock->base, ww_class->mutex_name, &ww_class->mutex_key);
+	__mutex_init(&lock->base, ww_class->mutex_name, &ww_class->mutex_key, 0);
	lock->ctx = NULL;
#ifdef CONFIG_DEBUG_MUTEXES
	lock->ww_class = ww_class;
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 803001a..b9f87a3 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -2009,7 +2009,7 @@ int stop_sync_thread(struct netns_ipvs *ipvs, int state)
 */
int __net_init ip_vs_sync_net_init(struct netns_ipvs *ipvs)
{
-	__mutex_init(&ipvs->sync_mutex, "ipvs->sync_mutex", &__ipvs_sync_key);
+	__mutex_init(&ipvs->sync_mutex, "ipvs->sync_mutex", &__ipvs_sync_key, 0);
	spin_lock_init(&ipvs->sync_lock);
	spin_lock_init(&ipvs->sync_buff_lock);
	return 0;
-- 
2.7.4

[toc] | [next] | [standalone]

#1424801

On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin wrote:
> Hello!
> 
>   To my surprise I found out that it's not possible to initialise a mutex into
>   a locked state.
>   I discussed it with Arjan and apparently there's no fundamental reason
>   not to allow this.

There is. A mutex _must_ have an owner. If you can initialize it in
locked state, you could do so statically, ie. outside of the context of
a task.

[toc] | [prev] | [next] | [standalone]

#1425125

On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:

> On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin wrote:
>> Hello!
>> 
>>  To my surprise I found out that it's not possible to initialise a mutex into
>>  a locked state.
>>  I discussed it with Arjan and apparently there's no fundamental reason
>>  not to allow this.
> 
> There is. A mutex _must_ have an owner. If you can initialize it in
> locked state, you could do so statically, ie. outside of the context of
> a task.

What's wrong with disallowing only static initializers, but allowing dynamic ones?
Then there is a clear owner.

[toc] | [prev] | [next] | [standalone]

#1425135

On Fri, Jun 17, 2016 at 10:14:10AM -0400, Oleg Drokin wrote:
> 
> On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:
> 
> > On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin wrote:
> >> Hello!
> >> 
> >>  To my surprise I found out that it's not possible to initialise a mutex into
> >>  a locked state.
> >>  I discussed it with Arjan and apparently there's no fundamental reason
> >>  not to allow this.
> > 
> > There is. A mutex _must_ have an owner. If you can initialize it in
> > locked state, you could do so statically, ie. outside of the context of
> > a task.
> 
> What's wrong with disallowing only static initializers, but allowing dynamic ones?
> Then there is a clear owner.

At which point, what wrong with the simple:

	mutex_init(&m);
	mutex_lock(&m);

Sequence? Its obvious, has clear semantics and doesn't extend the API.

[toc] | [prev] | [next] | [standalone]

#1425147

On Jun 17, 2016, at 10:19 AM, Peter Zijlstra wrote:

> On Fri, Jun 17, 2016 at 10:14:10AM -0400, Oleg Drokin wrote:
>> 
>> On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:
>> 
>>> On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin wrote:
>>>> Hello!
>>>> 
>>>> To my surprise I found out that it's not possible to initialise a mutex into
>>>> a locked state.
>>>> I discussed it with Arjan and apparently there's no fundamental reason
>>>> not to allow this.
>>> 
>>> There is. A mutex _must_ have an owner. If you can initialize it in
>>> locked state, you could do so statically, ie. outside of the context of
>>> a task.
>> 
>> What's wrong with disallowing only static initializers, but allowing dynamic ones?
>> Then there is a clear owner.
> 
> At which point, what wrong with the simple:
> 
> 	mutex_init(&m);
> 	mutex_lock(&m);
> 
> Sequence? Its obvious, has clear semantics and doesn't extend the API.

The problem is:

spin_lock(somelock);
structure = some_internal_list_lookup(list);
if (structure)
	goto out;

init_new_structure(new_structure);
mutex_init(&new_structure->s_mutex);
mutex_lock(&new_structure->s_mutex);  // XXX CANNOT DO THIS UNDER SPINLOCK!

list_add(list, new_structure->s_list);
structure = new_structure;
out:
spin_unlock(somelock);
return structure;

[toc] | [prev] | [next] | [standalone]

#1425162

On Fri, Jun 17, 2016 at 10:24:32AM -0400, Oleg Drokin wrote:
> 
> On Jun 17, 2016, at 10:19 AM, Peter Zijlstra wrote:
> 
> > On Fri, Jun 17, 2016 at 10:14:10AM -0400, Oleg Drokin wrote:
> >> 
> >> On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:
> >> 
> >>> On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin wrote:
> >>>> Hello!
> >>>> 
> >>>> To my surprise I found out that it's not possible to initialise a mutex into
> >>>> a locked state.
> >>>> I discussed it with Arjan and apparently there's no fundamental reason
> >>>> not to allow this.
> >>> 
> >>> There is. A mutex _must_ have an owner. If you can initialize it in
> >>> locked state, you could do so statically, ie. outside of the context of
> >>> a task.
> >> 
> >> What's wrong with disallowing only static initializers, but allowing dynamic ones?
> >> Then there is a clear owner.
> > 
> > At which point, what wrong with the simple:
> > 
> > 	mutex_init(&m);
> > 	mutex_lock(&m);
> > 
> > Sequence? Its obvious, has clear semantics and doesn't extend the API.
> 
> The problem is:
> 
> spin_lock(somelock);
> structure = some_internal_list_lookup(list);
> if (structure)
> 	goto out;
> 
> init_new_structure(new_structure);
> mutex_init(&new_structure->s_mutex);
> mutex_lock(&new_structure->s_mutex);  // XXX CANNOT DO THIS UNDER SPINLOCK!

	mutex_trylock(&new_structure->s_mutex);

should work, since you know it cannot be acquired yet by anybody else,
since you've not published it yet.

And a trylock does not sleep, so is perfectly fine under a spinlock.

> 
> list_add(list, new_structure->s_list);
> structure = new_structure;
> out:
> spin_unlock(somelock);
> return structure;
> 

[toc] | [prev] | [next] | [standalone]

#1425171

On Jun 17, 2016, at 10:32 AM, Peter Zijlstra wrote:

> On Fri, Jun 17, 2016 at 10:24:32AM -0400, Oleg Drokin wrote:
>> 
>> On Jun 17, 2016, at 10:19 AM, Peter Zijlstra wrote:
>> 
>>> On Fri, Jun 17, 2016 at 10:14:10AM -0400, Oleg Drokin wrote:
>>>> 
>>>> On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:
>>>> 
>>>>> On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin wrote:
>>>>>> Hello!
>>>>>> 
>>>>>> To my surprise I found out that it's not possible to initialise a mutex into
>>>>>> a locked state.
>>>>>> I discussed it with Arjan and apparently there's no fundamental reason
>>>>>> not to allow this.
>>>>> 
>>>>> There is. A mutex _must_ have an owner. If you can initialize it in
>>>>> locked state, you could do so statically, ie. outside of the context of
>>>>> a task.
>>>> 
>>>> What's wrong with disallowing only static initializers, but allowing dynamic ones?
>>>> Then there is a clear owner.
>>> 
>>> At which point, what wrong with the simple:
>>> 
>>> 	mutex_init(&m);
>>> 	mutex_lock(&m);
>>> 
>>> Sequence? Its obvious, has clear semantics and doesn't extend the API.
>> 
>> The problem is:
>> 
>> spin_lock(somelock);
>> structure = some_internal_list_lookup(list);
>> if (structure)
>> 	goto out;
>> 
>> init_new_structure(new_structure);
>> mutex_init(&new_structure->s_mutex);
>> mutex_lock(&new_structure->s_mutex);  // XXX CANNOT DO THIS UNDER SPINLOCK!
> 
> 	mutex_trylock(&new_structure->s_mutex);
> 
> should work, since you know it cannot be acquired yet by anybody else,
> since you've not published it yet.

This does work, but suddenly does not look so obvious anymore, does it?
I got some feedback that doing this is not really preferred.

Also once __must_check is added to mutex_try_lock() (surprised it's not yet),
we'll need to also have the useless "but what if it did fail to lock" path?

> And a trylock does not sleep, so is perfectly fine under a spinlock.
> 
>> 
>> list_add(list, new_structure->s_list);
>> structure = new_structure;
>> out:
>> spin_unlock(somelock);
>> return structure;
>> 

[toc] | [prev] | [next] | [standalone]

#1425173

On Fri, 2016-06-17 at 10:40 -0400, Oleg Drokin wrote:
> On Jun 17, 2016, at 10:32 AM, Peter Zijlstra wrote:
> 
> > 
> > On Fri, Jun 17, 2016 at 10:24:32AM -0400, Oleg Drokin wrote:
> > > 
> > > 
> > > On Jun 17, 2016, at 10:19 AM, Peter Zijlstra wrote:
> > > 
> > > > 
> > > > On Fri, Jun 17, 2016 at 10:14:10AM -0400, Oleg Drokin wrote:
> > > > > 
> > > > > 
> > > > > On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:
> > > > > 
> > > > > > 
> > > > > > On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin
> > > > > > wrote:
> > > > > > > 
> > > > > > > Hello!
> > > > > > > 
> > > > > > > To my surprise I found out that it's not possible to
> > > > > > > initialise a mutex into
> > > > > > > a locked state.
> > > > > > > I discussed it with Arjan and apparently there's no
> > > > > > > fundamental reason
> > > > > > > not to allow this.
> > > > > > There is. A mutex _must_ have an owner. If you can
> > > > > > initialize it in
> > > > > > locked state, you could do so statically, ie. outside of
> > > > > > the context of
> > > > > > a task.
> > > > > What's wrong with disallowing only static initializers, but
> > > > > allowing dynamic ones?
> > > > > Then there is a clear owner.
> > > > At which point, what wrong with the simple:
> > > > 
> > > > 	mutex_init(&m);
> > > > 	mutex_lock(&m);
> > > > 
> > > > Sequence? Its obvious, has clear semantics and doesn't extend
> > > > the API.
> > > The problem is:
> > > 
> > > spin_lock(somelock);
> > > structure = some_internal_list_lookup(list);
> > > if (structure)
> > > 	goto out;
> > > 
> > > init_new_structure(new_structure);
> > > mutex_init(&new_structure->s_mutex);
> > > mutex_lock(&new_structure->s_mutex);  // XXX CANNOT DO THIS UNDER
> > > SPINLOCK!
> > 	mutex_trylock(&new_structure->s_mutex);
> > 
> > should work, since you know it cannot be acquired yet by anybody
> > else,
> > since you've not published it yet.
> This does work, but suddenly does not look so obvious anymore, does
> it?
> I got some feedback that doing this is not really preferred.
> 
> Also once __must_check is added to mutex_try_lock() (surprised it's
> not yet),
> we'll need to also have the useless "but what if it did fail to lock"
> path?
> 

Maybe just BUG in that case, and add a comment that says something
along the lines of "this should always work since it's not hashed yet"
?
 
> > 
> > And a trylock does not sleep, so is perfectly fine under a
> > spinlock.
> > 
> > > 
> > > 
> > > list_add(list, new_structure->s_list);
> > > structure = new_structure;
> > > out:
> > > spin_unlock(somelock);
> > > return structure;
> > > 

-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1425180

On Jun 17, 2016, at 10:42 AM, Jeff Layton wrote:

> On Fri, 2016-06-17 at 10:40 -0400, Oleg Drokin wrote:
>> On Jun 17, 2016, at 10:32 AM, Peter Zijlstra wrote:
>> 
>>> 
>>> On Fri, Jun 17, 2016 at 10:24:32AM -0400, Oleg Drokin wrote:
>>>> 
>>>> 
>>>> On Jun 17, 2016, at 10:19 AM, Peter Zijlstra wrote:
>>>> 
>>>>> 
>>>>> On Fri, Jun 17, 2016 at 10:14:10AM -0400, Oleg Drokin wrote:
>>>>>> 
>>>>>> 
>>>>>> On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:
>>>>>> 
>>>>>>> 
>>>>>>> On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin
>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> Hello!
>>>>>>>> 
>>>>>>>> To my surprise I found out that it's not possible to
>>>>>>>> initialise a mutex into
>>>>>>>> a locked state.
>>>>>>>> I discussed it with Arjan and apparently there's no
>>>>>>>> fundamental reason
>>>>>>>> not to allow this.
>>>>>>> There is. A mutex _must_ have an owner. If you can
>>>>>>> initialize it in
>>>>>>> locked state, you could do so statically, ie. outside of
>>>>>>> the context of
>>>>>>> a task.
>>>>>> What's wrong with disallowing only static initializers, but
>>>>>> allowing dynamic ones?
>>>>>> Then there is a clear owner.
>>>>> At which point, what wrong with the simple:
>>>>> 
>>>>> 	mutex_init(&m);
>>>>> 	mutex_lock(&m);
>>>>> 
>>>>> Sequence? Its obvious, has clear semantics and doesn't extend
>>>>> the API.
>>>> The problem is:
>>>> 
>>>> spin_lock(somelock);
>>>> structure = some_internal_list_lookup(list);
>>>> if (structure)
>>>> 	goto out;
>>>> 
>>>> init_new_structure(new_structure);
>>>> mutex_init(&new_structure->s_mutex);
>>>> mutex_lock(&new_structure->s_mutex);  // XXX CANNOT DO THIS UNDER
>>>> SPINLOCK!
>>> 	mutex_trylock(&new_structure->s_mutex);
>>> 
>>> should work, since you know it cannot be acquired yet by anybody
>>> else,
>>> since you've not published it yet.
>> This does work, but suddenly does not look so obvious anymore, does
>> it?
>> I got some feedback that doing this is not really preferred.
>> 
>> Also once __must_check is added to mutex_try_lock() (surprised it's
>> not yet),
>> we'll need to also have the useless "but what if it did fail to lock"
>> path?
>> 
> 
> Maybe just BUG in that case, and add a comment that says something
> along the lines of "this should always work since it's not hashed yet"
> ?

Yes, we can add all sorts of checks that have various impacts on code readability,
we can also move code around that also have code readability and CPU impact.

But in my discussion with Arjan he said this is a new use case that was not met before
and suggested to mail it to the list.

>>> And a trylock does not sleep, so is perfectly fine under a
>>> spinlock.
>>> 
>>>> 
>>>> 
>>>> list_add(list, new_structure->s_list);
>>>> structure = new_structure;
>>>> out:
>>>> spin_unlock(somelock);
>>>> return structure;
>>>> 
> 
> -- 
> Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1425182

On 6/17/2016 7:54 AM, Oleg Drokin wrote:
>
> Yes, we can add all sorts of checks that have various impacts on code readability,
> we can also move code around that also have code readability and CPU impact.
>
> But in my discussion with Arjan he said this is a new use case that was not met before
> and suggested to mail it to the list.

I'm all in favor of having "end code" be as clear as possible wrt intent.
(and I will admit this is an curious use case, but not an insane silly one)

one other option is to make a wrapper

mutex_init_locked( )
{
	mutex_init()
	mutex_trylock()
}

that way the wrapper can be an inline in a header, but doesn't need to touch a wide
berth of stuff... while keeping the end code clear wrt intent

[toc] | [prev] | [next] | [standalone]

#1425186

On Fri, Jun 17, 2016 at 10:40:14AM -0400, Oleg Drokin wrote:

> >> The problem is:
> >> 
> >> spin_lock(somelock);
> >> structure = some_internal_list_lookup(list);
> >> if (structure)
> >> 	goto out;
> >> 
> >> init_new_structure(new_structure);
> >> mutex_init(&new_structure->s_mutex);
> >> mutex_lock(&new_structure->s_mutex);  // XXX CANNOT DO THIS UNDER SPINLOCK!
> > 
> > 	mutex_trylock(&new_structure->s_mutex);
> > 
> > should work, since you know it cannot be acquired yet by anybody else,
> > since you've not published it yet.
> 
> This does work, but suddenly does not look so obvious anymore, does it?

Well, the whole thing is somewhat tricky and would require a comment
anyway.

> I got some feedback that doing this is not really preferred.

Much preferred to adding additional API I would think. Because if we add
it here, we'll also have to add it to rt_mutex.

Also, I'm not sure I want to promote this construct, it seems like a
very special case.

> Also once __must_check is added to mutex_try_lock() (surprised it's not yet),
> we'll need to also have the useless "but what if it did fail to lock" path?

The BUG_ON() suggested by Jeff seems like a good solution ;-)

[toc] | [prev] | [next] | [standalone]

#1425172

The problem is if you are not holding the spinlock, some parallel thread might have added a structure into the
list that makes our no longer needed. So upon spinlock reacquisition we'll need to do another lookup, find
the now added structure and throw ours away - even more expensive.

On Jun 17, 2016, at 10:32 AM, Matthew Jacob wrote:

> At the risk of sounding annoying, the problem here isn't in the locking but in the flow.
> If the structure already exists, spinlock is fine. Return after dropping spinlock.
> 
> If the structure doesn't exist, you can drop the spinlock to create it (and initialize the lock &and& grab the lock, should you want to) *prior* to adding it to the list because nobody else could know about it until you put it on the list.
> 
> On Fri, Jun 17, 2016 at 7:24 AM, Oleg Drokin <green@linuxhacker.ru> wrote:
> 
> On Jun 17, 2016, at 10:19 AM, Peter Zijlstra wrote:
> 
> > On Fri, Jun 17, 2016 at 10:14:10AM -0400, Oleg Drokin wrote:
> >>
> >> On Jun 17, 2016, at 4:25 AM, Peter Zijlstra wrote:
> >>
> >>> On Wed, Jun 15, 2016 at 02:23:35PM -0400, Oleg Drokin wrote:
> >>>> Hello!
> >>>>
> >>>> To my surprise I found out that it's not possible to initialise a mutex into
> >>>> a locked state.
> >>>> I discussed it with Arjan and apparently there's no fundamental reason
> >>>> not to allow this.
> >>>
> >>> There is. A mutex _must_ have an owner. If you can initialize it in
> >>> locked state, you could do so statically, ie. outside of the context of
> >>> a task.
> >>
> >> What's wrong with disallowing only static initializers, but allowing dynamic ones?
> >> Then there is a clear owner.
> >
> > At which point, what wrong with the simple:
> >
> >       mutex_init(&m);
> >       mutex_lock(&m);
> >
> > Sequence? Its obvious, has clear semantics and doesn't extend the API.
> 
> The problem is:
> 
> spin_lock(somelock);
> structure = some_internal_list_lookup(list);
> if (structure)
>         goto out;
> 
> init_new_structure(new_structure);
> mutex_init(&new_structure->s_mutex);
> mutex_lock(&new_structure->s_mutex);  // XXX CANNOT DO THIS UNDER SPINLOCK!
> 
> list_add(list, new_structure->s_list);
> structure = new_structure;
> out:
> spin_unlock(somelock);
> return structure;
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web