All of lore.kernel.org
 help / color / mirror / Atom feed
From: Waiman Long <waiman.long@hp.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Jeff Layton <jlayton@redhat.com>,
	Miklos Szeredi <mszeredi@suse.cz>, Ingo Molnar <mingo@redhat.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Andi Kleen <andi@firstfloor.org>,
	"Chandramouleeswaran, Aswin" <aswin@hp.com>,
	"Norton, Scott J" <scott.norton@hp.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH v2 1/2] spinlock: New spinlock_refcount.h for lockless update of refcount
Date: Sat, 29 Jun 2013 16:23:10 -0400	[thread overview]
Message-ID: <51CF422E.7030803@hp.com> (raw)
In-Reply-To: <CA+55aFzo5Jy064pbvEZh5cg_RJiSxe1R+tSgByQTs4rc=4SN3Q@mail.gmail.com>

On 06/29/2013 01:45 PM, Linus Torvalds wrote:
> Sorry for not commenting earlier, I was traveling and keeping email to
> a minimum..
>
> On Wed, Jun 26, 2013 at 10:43 AM, Waiman Long<Waiman.Long@hp.com>  wrote:
>> This patch introduces a new spinlock_refcount.h header file to be
>> included by kernel code that want to do a lockless update of reference
>> count protected by a spinlock.
> So I really like the concept, but the implementation is a mess, and
> tries to do too much, while actually achieving too little.
>
> I do not believe you should care about debug spinlocks at all, and
> just leave them be. Have a simple fallback code that defaults to
> regular counts and spinlocks, and have any debug cases just use that.

I was concern that people might want to have the same behavior even when 
spinlock debugging was  on. Apparently, this is not really needed. Now I 
can just disable the optimization and fall back to the old path when 
spinlock debugging is on.

> But more importantly, I think this needs to be architecture-specific,
> and using<linux/spinlock_refcount.h>  to try to do some generic 64-bit
> cmpxchg() version is a bad bad idea.

Yes, I can put the current implementation into 
asm-generic/spinlock_refcount.h. Now I need to put an 
asm/spinlock_refcount.h into every arch's include/asm directory. Right? 
I don't think there is a mechanism in the build script to create a 
symlink from asm to generic-asm when a header file is missing. Is it the 
general rule that we should have a linux/spinlock_refcount.h that 
include asm/spinlock_refcount.h instead of including 
asm/spinlock_refcount.h directly?

> We have several architectures coming up that have memory transaction
> support, and the "spinlock with refcount" is a perfect candidate for a
> transactional memory implementation. So when introducing a new atomic
> like this that is very performance-critical and used for some very
> core code, I really think architectures would want to make their own
> optimized versions.
>
> These things should also not be inlined, I think.
>
> So I think the concept is good, but I think the implementation needs
> more thought.
>
>                         Linus

Thank for the comment. I will try to come up with a version that is 
acceptable to all stakeholders.

Regards,
Longman


  reply	other threads:[~2013-06-29 20:23 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-26 17:43 [PATCH v2 0/2] Lockless update of reference count protected by spinlock Waiman Long
2013-06-26 17:43 ` [PATCH v2 1/2] spinlock: New spinlock_refcount.h for lockless update of refcount Waiman Long
2013-06-26 20:17   ` Andi Kleen
2013-06-26 21:07     ` Waiman Long
2013-06-26 21:22       ` Andi Kleen
2013-06-26 23:26         ` Waiman Long
2013-06-27  1:06           ` Andi Kleen
2013-06-27  1:15             ` Waiman Long
2013-06-27  1:24               ` Waiman Long
2013-06-27  1:37                 ` Andi Kleen
2013-06-27 14:56                   ` Waiman Long
2013-06-28 13:46                     ` Thomas Gleixner
2013-06-29 20:30                       ` Waiman Long
2013-06-26 23:27         ` Thomas Gleixner
2013-06-26 23:06   ` Thomas Gleixner
2013-06-27  0:16     ` Waiman Long
2013-06-27 14:44       ` Thomas Gleixner
2013-06-29 21:03         ` Waiman Long
2013-06-27  0:26     ` Waiman Long
2013-06-29 17:45   ` Linus Torvalds
2013-06-29 20:23     ` Waiman Long [this message]
2013-06-29 21:34       ` Waiman Long
2013-06-29 22:11         ` Linus Torvalds
2013-06-29 22:34           ` Waiman Long
2013-06-29 21:58       ` Linus Torvalds
2013-06-29 22:47         ` Linus Torvalds
2013-07-01 13:40           ` Waiman Long
2013-06-26 17:43 ` [PATCH v2 2/2] dcache: Locklessly update d_count whenever possible Waiman Long

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=51CF422E.7030803@hp.com \
    --to=waiman.long@hp.com \
    --cc=andi@firstfloor.org \
    --cc=aswin@hp.com \
    --cc=benh@kernel.crashing.org \
    --cc=jlayton@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=mszeredi@suse.cz \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=scott.norton@hp.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /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.