Messages in this thread Patch in this message | | | Date | Fri, 6 Feb 2015 22:18:09 +0000 (UTC) | From | Mathieu Desnoyers <> | Subject | Re: [PATCH] llist: Fix missing memory barrier |
| |
----- Original Message ----- > From: "Peter Hurley" <peter@hurleysoftware.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, "Pranith Kumar" <bobby.prani@gmail.com> > Cc: "Huang Ying" <ying.huang@intel.com>, "LKML" <linux-kernel@vger.kernel.org>, "Paul McKenney" > <paulmck@linux.vnet.ibm.com>, "David Howells" <dhowells@redhat.com>, "Greg Kroah-Hartman" > <gregkh@linuxfoundation.org> > Sent: Friday, February 6, 2015 10:17:37 AM > Subject: Re: [PATCH] llist: Fix missing memory barrier > > On 02/06/2015 09:12 AM, Mathieu Desnoyers wrote: > > ----- Original Message ----- > >> From: "Pranith Kumar" <bobby.prani@gmail.com> > >> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >> Cc: "Huang Ying" <ying.huang@intel.com>, "LKML" > >> <linux-kernel@vger.kernel.org>, "Paul McKenney" > >> <paulmck@linux.vnet.ibm.com>, "David Howells" <dhowells@redhat.com> > >> Sent: Thursday, February 5, 2015 10:44:07 PM > >> Subject: Re: [PATCH] llist: Fix missing memory barrier > >> > >> Hi Mathieu, > >> > >> On Thu, Feb 5, 2015 at 10:06 PM, Mathieu Desnoyers > >> <mathieu.desnoyers@efficios.com> wrote: > >>> A smp_read_barrier_depends() appears to be missing in llist_del_first(). > >>> It should only matter for Alpha in practice. Adding it after the check > >>> of entry against NULL allows skipping the barrier in a common case. > >> > >> We recently decided on using lockless_dereference() instead of > >> hard-coding smp_read_barrier_depends()[1]. The advantage is that > >> lockless_dereference() clearly shows what loads are being ordered. > >> Could you resend the patch using that API? > > > > Since llist.h has been introduced prior to 3.18, I'm wondering if > > it would be worthwhile to submit 2 patches for the purpose of > > backporting to stable branches: > > > > 1) Fix introducing smp_read_barrier_depends() (for master and > > stable branches) > > 2) Move master from smp_read_barrier_depends() to > > lockless_dereference(), > > > > Thoughts ? > > Other way around. > > The first patch should use lockless_dereference() for mainline. > > Then once that's been picked up and has a SHA, then a backport > patch for stable using smp_read_barrier_depends() instead > before lockless_dereference() was introduced.
That sounds like a good approach. I'll wait for a final word from Greg and proceed this way unless there are objections.
Thanks!
Mathieu
> > Regards, > Peter Hurley > > > Thanks! > > > > Mathieu > > > > > >> > >> Thanks! > >> > >> [1] http://lkml.iu.edu/hypermail/linux/kernel/1410.3/04561.html > >> > >>> > >>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > >>> CC: Huang Ying <ying.huang@intel.com> > >>> CC: Paul McKenney <paulmck@linux.vnet.ibm.com> > >>> CC: David Howells <dhowells@redhat.com> > >>> --- > >>> lib/llist.c | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >>> diff --git a/lib/llist.c b/lib/llist.c > >>> index f76196d..72861f3 100644 > >>> --- a/lib/llist.c > >>> +++ b/lib/llist.c > >>> @@ -26,6 +26,7 @@ > >>> #include <linux/export.h> > >>> #include <linux/interrupt.h> > >>> #include <linux/llist.h> > >>> +#include <asm/barrier.h> > >>> > >>> > >>> /** > >>> @@ -72,6 +73,12 @@ struct llist_node *llist_del_first(struct llist_head > >>> *head) > >>> if (entry == NULL) > >>> return NULL; > >>> old_entry = entry; > >>> + /* > >>> + * Load entry before entry->next. Matches the implicit > >>> + * memory barrier before the cmpxchg in > >>> llist_add_batch(), > >>> + * which ensures entry->next is stored before entry. > >>> + */ > >>> + smp_read_barrier_depends(); > >>> next = entry->next; > >>> entry = cmpxchg(&head->first, old_entry, next); > >>> if (entry == old_entry) > >>> -- > >>> 2.1.4 > >>> > >>> -- > >>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" > >>> in > >>> the body of a message to majordomo@vger.kernel.org > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > >>> Please read the FAQ at http://www.tux.org/lkml/ > >> > >> > >> > >> -- > >> Pranith > >> > > > >
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |