All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Cc: linux-arch@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org, robclark@gmail.com,
	rostedt@goodmis.org, Dave Airlie <airlied@redhat.com>,
	tglx@linutronix.de, mingo@elte.hu, linux-media@vger.kernel.org
Subject: Re: [PATCH v3 2/3] mutex: add support for wound/wait style locks, v3
Date: Mon, 27 May 2013 10:21:49 +0200	[thread overview]
Message-ID: <20130527082149.GE2781@laptop> (raw)
In-Reply-To: <519CFF56.90600@canonical.com>

On Wed, May 22, 2013 at 07:24:38PM +0200, Maarten Lankhorst wrote:
> >> +static inline void ww_acquire_init(struct ww_acquire_ctx *ctx,
> >> +				   struct ww_class *ww_class)
> >> +{
> >> +	ctx->task = current;
> >> +	do {
> >> +		ctx->stamp = atomic_long_inc_return(&ww_class->stamp);
> >> +	} while (unlikely(!ctx->stamp));
> > I suppose we'll figure something out when this becomes a bottleneck. Ideally
> > we'd do something like:
> >
> >  ctx->stamp = local_clock();
> >
> > but for now we cannot guarantee that's not jiffies, and I suppose that's a tad
> > too coarse to work for this.
> This might mess up when 2 cores happen to return exactly the same time, how do you choose a winner in that case?
> EDIT: Using pointer address like you suggested below is fine with me. ctx pointer would be static enough.

Right, but for now I suppose the 'global' atomic is ok, if/when we find
it hurts performance we can revisit. I was just spewing ideas :-)

> > Also, why is 0 special?
> Oops, 0 is no longer special.
> 
> I used to set the samp directly on the lock, so 0 used to mean no ctx set.

Ah, ok :-)

> >> +static inline int __must_check ww_mutex_trylock_single(struct ww_mutex *lock)
> >> +{
> >> +	return mutex_trylock(&lock->base);
> >> +}
> > trylocks can never deadlock they don't block per definition, I don't see the
> > point of the _single() thing here.
> I called it single because they weren't annotated into any ctx. I can drop the _single suffix though,
> but you'd still need to unlock with unlock_single, or we need to remove that distinction altogether,
> lose a few lockdep checks and only have a one unlock function.

Again, early.. monday.. would a trylock, even if successful still need
the ctx?

WARNING: multiple messages have this Message-ID (diff)
From: Peter Zijlstra <peterz@infradead.org>
To: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Cc: linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
	x86@kernel.org, dri-devel@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org, robclark@gmail.com,
	rostedt@goodmis.org, tglx@linutronix.de, mingo@elte.hu,
	linux-media@vger.kernel.org, Dave Airlie <airlied@redhat.com>
Subject: Re: [PATCH v3 2/3] mutex: add support for wound/wait style locks, v3
Date: Mon, 27 May 2013 10:21:49 +0200	[thread overview]
Message-ID: <20130527082149.GE2781@laptop> (raw)
Message-ID: <20130527082149.C6yBfDH1hqlRvTDYt8TowhnmZb7BbpSFXWziRA7sgvM@z> (raw)
In-Reply-To: <519CFF56.90600@canonical.com>

On Wed, May 22, 2013 at 07:24:38PM +0200, Maarten Lankhorst wrote:
> >> +static inline void ww_acquire_init(struct ww_acquire_ctx *ctx,
> >> +				   struct ww_class *ww_class)
> >> +{
> >> +	ctx->task = current;
> >> +	do {
> >> +		ctx->stamp = atomic_long_inc_return(&ww_class->stamp);
> >> +	} while (unlikely(!ctx->stamp));
> > I suppose we'll figure something out when this becomes a bottleneck. Ideally
> > we'd do something like:
> >
> >  ctx->stamp = local_clock();
> >
> > but for now we cannot guarantee that's not jiffies, and I suppose that's a tad
> > too coarse to work for this.
> This might mess up when 2 cores happen to return exactly the same time, how do you choose a winner in that case?
> EDIT: Using pointer address like you suggested below is fine with me. ctx pointer would be static enough.

Right, but for now I suppose the 'global' atomic is ok, if/when we find
it hurts performance we can revisit. I was just spewing ideas :-)

> > Also, why is 0 special?
> Oops, 0 is no longer special.
> 
> I used to set the samp directly on the lock, so 0 used to mean no ctx set.

Ah, ok :-)

> >> +static inline int __must_check ww_mutex_trylock_single(struct ww_mutex *lock)
> >> +{
> >> +	return mutex_trylock(&lock->base);
> >> +}
> > trylocks can never deadlock they don't block per definition, I don't see the
> > point of the _single() thing here.
> I called it single because they weren't annotated into any ctx. I can drop the _single suffix though,
> but you'd still need to unlock with unlock_single, or we need to remove that distinction altogether,
> lose a few lockdep checks and only have a one unlock function.

Again, early.. monday.. would a trylock, even if successful still need
the ctx?



  parent reply	other threads:[~2013-05-27  8:21 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-28 17:03 [PATCH v3 0/3] Wait/wound mutex implementation, v3 Maarten Lankhorst
2013-04-28 17:03 ` [PATCH v3 1/3] arch: make __mutex_fastpath_lock_retval return whether fastpath succeeded or not Maarten Lankhorst
2013-04-28 17:04 ` [PATCH v3 2/3] mutex: add support for wound/wait style locks, v3 Maarten Lankhorst
2013-04-30 19:14   ` Daniel Vetter
2013-05-22 11:18     ` Maarten Lankhorst
2013-05-22 11:37       ` Peter Zijlstra
2013-05-22 11:47         ` Maarten Lankhorst
2013-05-22 12:07           ` Peter Zijlstra
2013-05-22 16:18       ` Peter Zijlstra
2013-05-22 16:49         ` Daniel Vetter
2013-05-22 16:49           ` Daniel Vetter
2013-05-27  8:29           ` Peter Zijlstra
2013-05-22 17:24         ` Maarten Lankhorst
2013-05-23  9:13           ` Maarten Lankhorst
2013-05-23 10:45             ` [Linaro-mm-sig] " Daniel Vetter
2013-05-27  8:00           ` Peter Zijlstra
2013-05-27  8:26             ` Maarten Lankhorst
2013-05-27  9:13               ` Peter Zijlstra
2013-05-27  9:58                 ` Maarten Lankhorst
2013-05-27  8:21           ` Peter Zijlstra [this message]
2013-05-27  8:21             ` Peter Zijlstra
2013-05-27 10:01             ` Maarten Lankhorst
2013-05-27 10:24               ` Peter Zijlstra
2013-05-27 10:52                 ` Maarten Lankhorst
2013-05-27 11:15                   ` Peter Zijlstra
2013-05-27 11:24                     ` Maarten Lankhorst
2013-05-27 14:47             ` Daniel Vetter
2013-05-27 14:55               ` Daniel Vetter
2013-04-28 17:04 ` [PATCH v3 3/3] mutex: Add ww tests to lib/locking-selftest.c. v3 Maarten Lankhorst
2013-04-30 18:45 ` [PATCH] [RFC] mutex: w/w mutex slowpath debugging Daniel Vetter
2013-04-30 19:29   ` Steven Rostedt
2013-04-30 20:38     ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130527082149.GE2781@laptop \
    --to=peterz@infradead.org \
    --cc=airlied@redhat.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=maarten.lankhorst@canonical.com \
    --cc=mingo@elte.hu \
    --cc=robclark@gmail.com \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.