Messages in this thread | | | Date | Fri, 06 Feb 2015 10:17:37 -0500 | From | Peter Hurley <> | 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.
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 >> >
| |