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.
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
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?
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
----- 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 ?
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
On Fri, Feb 06, 2015 at 02:12:32PM +0000, 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 ?
Yes, why? What code needs these new apis?
confused,
greg k-h
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.
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
>>
>
----- Original Message -----
> From: "Greg Kroah-Hartman" <[email protected]>
> To: "Mathieu Desnoyers" <[email protected]>
> Cc: "Pranith Kumar" <[email protected]>, "Huang Ying" <[email protected]>, "LKML"
> <[email protected]>, "Paul McKenney" <[email protected]>, "David Howells" <[email protected]>
> Sent: Friday, February 6, 2015 10:03:59 AM
> Subject: Re: [PATCH] llist: Fix missing memory barrier
>
> On Fri, Feb 06, 2015 at 02:12:32PM +0000, 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 ?
>
> Yes, why? What code needs these new apis?
The subsystems using llist.h in master: IRQ, smp core code,
vmalloc, scsi, the block layer, some filesystems... and more.
grep for "llist.h" to see the complete list of users.
My question was mainly on how to do the fix process-wise: post-3.18,
it implies using the new lockless_dereference() API. pre-3.18, we
need to use smp_read_barrier_depends().
As Peter Hurley suggested, using the new API for master and 3.18
seems like the right approach. Then the backports to stable branches
can, if needed, use smp_read_barrier_depends() instead. Is that
OK with you ?
Thanks,
Mathieu
>
> confused,
>
> greg k-h
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
----- 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
On Fri, Feb 06, 2015 at 10:16:25PM +0000, Mathieu Desnoyers wrote:
> ----- Original Message -----
> > From: "Greg Kroah-Hartman" <[email protected]>
> > To: "Mathieu Desnoyers" <[email protected]>
> > Cc: "Pranith Kumar" <[email protected]>, "Huang Ying" <[email protected]>, "LKML"
> > <[email protected]>, "Paul McKenney" <[email protected]>, "David Howells" <[email protected]>
> > Sent: Friday, February 6, 2015 10:03:59 AM
> > Subject: Re: [PATCH] llist: Fix missing memory barrier
> >
> > On Fri, Feb 06, 2015 at 02:12:32PM +0000, 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 ?
> >
> > Yes, why? What code needs these new apis?
>
> The subsystems using llist.h in master: IRQ, smp core code,
> vmalloc, scsi, the block layer, some filesystems... and more.
> grep for "llist.h" to see the complete list of users.
>
> My question was mainly on how to do the fix process-wise: post-3.18,
> it implies using the new lockless_dereference() API. pre-3.18, we
> need to use smp_read_barrier_depends().
>
> As Peter Hurley suggested, using the new API for master and 3.18
> seems like the right approach. Then the backports to stable branches
> can, if needed, use smp_read_barrier_depends() instead. Is that
> OK with you ?
Sounds fine to me.
greg k-h