----- Original Message ----- > From: "Peter Hurley" <[email protected]> > To: "Mathieu Desnoyers" <[email protected]>, "Pranith Kumar" > <[email protected]> > Cc: "Huang Ying" <[email protected]>, "LKML" > <[email protected]>, "Paul McKenney" > <[email protected]>, "David Howells" <[email protected]>, "Greg > Kroah-Hartman" > <[email protected]> > 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" <[email protected]> > >> To: "Mathieu Desnoyers" <[email protected]> > >> Cc: "Huang Ying" <[email protected]>, "LKML" > >> <[email protected]>, "Paul McKenney" > >> <[email protected]>, "David Howells" <[email protected]> > >> 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 > >> <[email protected]> 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 <[email protected]> > >>> CC: Huang Ying <[email protected]> > >>> CC: Paul McKenney <[email protected]> > >>> CC: David Howells <[email protected]> > >>> --- > >>> 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 [email protected] > >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
