All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
To: gregkh@suse.de, akpm@linux-foundation.org
Cc: linux-kernel@vger.kernel.org, ostrikov@nvidia.com,
	adobriyan@gmail.com, eric.dumazet@gmail.com, mingo@elte.hu,
	Oliver Neukum <oneukum@suse.de>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>
Subject: [PATCH 3/3] kref: Remove the memory barriers
Date: Sat, 10 Dec 2011 11:43:44 +0100	[thread overview]
Message-ID: <20111210104840.295857663@chello.nl> (raw)
In-Reply-To: 20111210104341.592561407@chello.nl

[-- Attachment #1: kref-simplify-2.patch --]
[-- Type: text/plain, Size: 1761 bytes --]

Commit 1b0b3b9980e ("kref: fix CPU ordering with respect to krefs")
wrongly adds memory barriers to kref.

It states:

  some atomic operations are only atomic, not ordered. Thus a CPU is allowed
  to reorder memory references to an object to before the reference is
  obtained. This fixes it.

While true, it fails to show why this is a problem. I say it is not a
problem because if there is a race with kref_put() such that we could
end up referencing a free'd object without this memory barrier, we
would still have that race with the memory barrier.

The kref_put() in question could complete (and free the object) before
the atomic_inc() and we'd still be up shit creek.

The kref_init() case is even worse, if your object is published at this
time you're so wrong the memory barrier won't make a difference what
so ever. If its not published, the act of publishing should include
the needed barriers/locks to make sure all writes prior to the act of
publishing are complete such that others will only observe a complete
object.

Cc: adobriyan@gmail.com
Cc: eric.dumazet@gmail.com
Cc: mingo@elte.hu
Cc: Oliver Neukum <oneukum@suse.de>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/kref.h |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6/include/linux/kref.h
===================================================================
--- linux-2.6.orig/include/linux/kref.h
+++ linux-2.6/include/linux/kref.h
@@ -28,7 +28,6 @@ struct kref {
 static inline void kref_init(struct kref *kref)
 {
 	atomic_set(&kref->refcount, 1);
-	smp_mb();
 }
 
 /**
@@ -39,7 +38,6 @@ static inline void kref_get(struct kref
 {
 	WARN_ON(!atomic_read(&kref->refcount));
 	atomic_inc(&kref->refcount);
-	smp_mb__after_atomic_inc();
 }
 
 /**



  parent reply	other threads:[~2011-12-10 10:53 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-12-10 10:43 [PATCH 0/3] kref: inline and barriers Peter Zijlstra
2011-12-10 10:43 ` [PATCH 1/3] kref: Inline all functions Peter Zijlstra
2011-12-10 14:32   ` Ming Lei
2011-12-10 14:59     ` Peter Zijlstra
2011-12-12 22:11   ` Greg KH
2011-12-13  9:36     ` Peter Zijlstra
2011-12-13 17:15       ` Greg KH
2011-12-13 18:52         ` Peter Zijlstra
2011-12-13 19:11           ` Greg KH
2011-12-13 19:36             ` Peter Zijlstra
2011-12-10 10:43 ` [PATCH 2/3] kref: Implement kref_put in terms of kref_sub Peter Zijlstra
2011-12-10 10:43 ` Peter Zijlstra [this message]
2011-12-10 14:07   ` [PATCH 3/3] kref: Remove the memory barriers Ming Lei
2011-12-10 14:58     ` Peter Zijlstra
2011-12-10 15:57       ` Ming Lei
2011-12-10 19:49         ` Peter Zijlstra
2011-12-11  2:22           ` Ming Lei
2011-12-11 12:47             ` Peter Zijlstra
2011-12-11 12:59               ` Ming Lei
2011-12-11 15:35                 ` Peter Zijlstra
2011-12-11 20:42                   ` Peter Zijlstra
2011-12-12  3:48                     ` Ming Lei
2011-12-12  8:54                       ` Peter Zijlstra
2011-12-12  9:57                         ` Ming Lei
2011-12-12 10:12                           ` Peter Zijlstra
2011-12-12 10:32                             ` Ming Lei
2011-12-12 11:05                               ` Peter Zijlstra
2011-12-12 11:19                                 ` Ming Lei
2011-12-12 11:13                               ` Eric Dumazet
2011-12-12 11:15                               ` Oliver Neukum
2011-12-12 10:20                           ` Oliver Neukum
2011-12-12 19:30                             ` Greg KH
2011-12-12 22:56                               ` Oliver Neukum
2011-12-12 23:14                                 ` Greg KH
2011-12-13 11:51                                   ` Oliver Neukum
2011-12-13  9:12                                 ` Peter Zijlstra
2011-12-13  9:49                                   ` Oliver Neukum
2011-12-12  8:55                       ` Peter Zijlstra
2011-12-12 15:24                         ` Greg KH
2011-12-12  8:56                       ` Peter Zijlstra
2011-12-12 10:10                         ` Ming Lei

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=20111210104840.295857663@chello.nl \
    --to=a.p.zijlstra@chello.nl \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=eric.dumazet@gmail.com \
    --cc=gregkh@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=oneukum@suse.de \
    --cc=ostrikov@nvidia.com \
    /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.