Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1538916 > unrolled thread

[RFC] llist: Fix code comments about llist_del_first locking

Started byJoel Fernandes <joelaf@google.com>
First post2016-12-08 23:10 +0100
Last post2016-12-09 01:50 +0100
Articles 7 — 2 participants

Back to article view | Back to linux.kernel


Contents

  [RFC] llist: Fix code comments about llist_del_first locking Joel Fernandes <joelaf@google.com> - 2016-12-08 23:10 +0100
    Re: [RFC] llist: Fix code comments about llist_del_first locking "Huang\, Ying" <ying.huang@intel.com> - 2016-12-09 01:40 +0100
      Re: [RFC] llist: Fix code comments about llist_del_first locking Joel Fernandes <joelaf@google.com> - 2016-12-09 01:50 +0100
        Re: [RFC] llist: Fix code comments about llist_del_first locking "Huang\, Ying" <ying.huang@intel.com> - 2016-12-09 03:20 +0100
          Re: [RFC] llist: Fix code comments about llist_del_first locking Joel Fernandes <joelaf@google.com> - 2016-12-09 03:30 +0100
            Re: [RFC] llist: Fix code comments about llist_del_first locking "Huang\, Ying" <ying.huang@intel.com> - 2016-12-09 03:30 +0100
      Re: [RFC] llist: Fix code comments about llist_del_first locking Joel Fernandes <joelaf@google.com> - 2016-12-09 01:50 +0100

#1538916 — [RFC] llist: Fix code comments about llist_del_first locking

FromJoel Fernandes <joelaf@google.com>
Date2016-12-08 23:10 +0100
Subject[RFC] llist: Fix code comments about llist_del_first locking
Message-ID<sMf34-3iT-49@gated-at.bofh.it>
Usage llist_del_first needs lock protection, however the table in the
comments of llist.h show a '-'. Correct this, and also add better
comments on top.

Cc: Huang Ying <ying.huang@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
 include/linux/llist.h | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/llist.h b/include/linux/llist.h
index fd4ca0b..15e4949 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -3,14 +3,15 @@
 /*
  * Lock-less NULL terminated single linked list
  *
- * If there are multiple producers and multiple consumers, llist_add
- * can be used in producers and llist_del_all can be used in
- * consumers.  They can work simultaneously without lock.  But
- * llist_del_first can not be used here.  Because llist_del_first
- * depends on list->first->next does not changed if list->first is not
- * changed during its operation, but llist_del_first, llist_add,
- * llist_add (or llist_del_all, llist_add, llist_add) sequence in
- * another consumer may violate that.
+ * If there are multiple producers and multiple consumers, llist_add can be
+ * used in producers and llist_del_all can be used in consumers.  They can work
+ * simultaneously without lock.  But llist_del_first will need to use a lock
+ * with any other operation (ABA problem).  This is because llist_del_first
+ * depends on list->first->next not changing but there's no way to be sure
+ * about that and the cmpxchg in llist_del_first may succeed if list->first is
+ * the same after concurrent operations. For example, a llist_del_first,
+ * llist_add, llist_add (or llist_del_all, llist_add, llist_add) sequence in
+ * another consumer may cause violations.
  *
  * If there are multiple producers and one consumer, llist_add can be
  * used in producers and llist_del_all or llist_del_first can be used
@@ -19,7 +20,7 @@
  * This can be summarized as follow:
  *
  *           |   add    | del_first |  del_all
- * add       |    -     |     -     |     -
+ * add       |    -     |     L     |     -
  * del_first |          |     L     |     L
  * del_all   |          |           |     -
  *
-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]


#1538987

From"Huang\, Ying" <ying.huang@intel.com>
Date2016-12-09 01:40 +0100
Message-ID<sMhoe-4BC-25@gated-at.bofh.it>
In reply to#1538916
Joel Fernandes <joelaf@google.com> writes:

> Usage llist_del_first needs lock protection, however the table in the
> comments of llist.h show a '-'. Correct this, and also add better
> comments on top.
>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Joel Fernandes <joelaf@google.com>
> ---
>  include/linux/llist.h | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/llist.h b/include/linux/llist.h
> index fd4ca0b..15e4949 100644
> --- a/include/linux/llist.h
> +++ b/include/linux/llist.h
> @@ -3,14 +3,15 @@
>  /*
>   * Lock-less NULL terminated single linked list
>   *
> - * If there are multiple producers and multiple consumers, llist_add
> - * can be used in producers and llist_del_all can be used in
> - * consumers.  They can work simultaneously without lock.  But
> - * llist_del_first can not be used here.  Because llist_del_first
> - * depends on list->first->next does not changed if list->first is not
> - * changed during its operation, but llist_del_first, llist_add,
> - * llist_add (or llist_del_all, llist_add, llist_add) sequence in
> - * another consumer may violate that.
> + * If there are multiple producers and multiple consumers, llist_add can be
> + * used in producers and llist_del_all can be used in consumers.  They can work
> + * simultaneously without lock.  But llist_del_first will need to use a lock
> + * with any other operation (ABA problem).  This is because llist_del_first
> + * depends on list->first->next not changing but there's no way to be sure
> + * about that and the cmpxchg in llist_del_first may succeed if list->first is
> + * the same after concurrent operations. For example, a llist_del_first,
> + * llist_add, llist_add (or llist_del_all, llist_add, llist_add) sequence in
> + * another consumer may cause violations.
>   *
>   * If there are multiple producers and one consumer, llist_add can be
>   * used in producers and llist_del_all or llist_del_first can be used
> @@ -19,7 +20,7 @@
>   * This can be summarized as follow:
>   *
>   *           |   add    | del_first |  del_all
> - * add       |    -     |     -     |     -
> + * add       |    -     |     L     |     -

If there are only one consumer which only calls llist_del_first(), lock
is unnecessary.  So '-' is shown here originally.  But if there are
multiple consumers which call llist_del_first() or llist_del_all(), lock
is needed.

Best Regards,
Huang, Ying

>   * del_first |          |     L     |     L
>   * del_all   |          |           |     -
>   *

[toc] | [prev] | [next] | [standalone]


#1538989

FromJoel Fernandes <joelaf@google.com>
Date2016-12-09 01:50 +0100
Message-ID<sMhxU-4EV-19@gated-at.bofh.it>
In reply to#1538987
On Thu, Dec 8, 2016 at 4:42 PM, Joel Fernandes <joelaf@google.com> wrote:
> On Thu, Dec 8, 2016 at 4:35 PM, Huang, Ying <ying.huang@intel.com> wrote:
>> Joel Fernandes <joelaf@google.com> writes:
>>
>>> Usage llist_del_first needs lock protection, however the table in the
>>> comments of llist.h show a '-'. Correct this, and also add better
>>> comments on top.
>>>
>>> Cc: Huang Ying <ying.huang@intel.com>
>>> Cc: Ingo Molnar <mingo@kernel.org>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
>>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>>> ---
>>>  include/linux/llist.h | 19 ++++++++++---------
>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>>> index fd4ca0b..15e4949 100644
>>> --- a/include/linux/llist.h
>>> +++ b/include/linux/llist.h
>>> @@ -3,14 +3,15 @@
>>>  /*
>>>   * Lock-less NULL terminated single linked list
>>>   *
>>> - * If there are multiple producers and multiple consumers, llist_add
>>> - * can be used in producers and llist_del_all can be used in
>>> - * consumers.  They can work simultaneously without lock.  But
>>> - * llist_del_first can not be used here.  Because llist_del_first
>>> - * depends on list->first->next does not changed if list->first is not
>>> - * changed during its operation, but llist_del_first, llist_add,
>>> - * llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>> - * another consumer may violate that.
>>> + * If there are multiple producers and multiple consumers, llist_add can be
>>> + * used in producers and llist_del_all can be used in consumers.  They can work
>>> + * simultaneously without lock.  But llist_del_first will need to use a lock
>>> + * with any other operation (ABA problem).  This is because llist_del_first
>>> + * depends on list->first->next not changing but there's no way to be sure
>>> + * about that and the cmpxchg in llist_del_first may succeed if list->first is
>>> + * the same after concurrent operations. For example, a llist_del_first,
>>> + * llist_add, llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>> + * another consumer may cause violations.
>>>   *
>>>   * If there are multiple producers and one consumer, llist_add can be
>>>   * used in producers and llist_del_all or llist_del_first can be used
>>> @@ -19,7 +20,7 @@
>>>   * This can be summarized as follow:
>>>   *
>>>   *           |   add    | del_first |  del_all
>>> - * add       |    -     |     -     |     -
>>> + * add       |    -     |     L     |     -
>>
>> If there are only one consumer which only calls llist_del_first(), lock
>> is unnecessary.  So '-' is shown here originally.  But if there are
>> multiple consumers which call llist_del_first() or llist_del_all(), lock
>> is needed.
>
> I think this needs to be made more clear in the table. The table
> doesn't clear say whether it describes the preceding paragraph
> (multiple producers and one consumer), or if it describes the multiple
> producers and one consumer case. So either we should have 2 tables, or

Sorry, I meant "or if it describes the multiple producer and multiple
consumer case".

Regards,
Joel

[toc] | [prev] | [next] | [standalone]


#1539011

From"Huang\, Ying" <ying.huang@intel.com>
Date2016-12-09 03:20 +0100
Message-ID<sMiWZ-5GC-5@gated-at.bofh.it>
In reply to#1538989
Joel Fernandes <joelaf@google.com> writes:

> On Thu, Dec 8, 2016 at 4:42 PM, Joel Fernandes <joelaf@google.com> wrote:
>> On Thu, Dec 8, 2016 at 4:35 PM, Huang, Ying <ying.huang@intel.com> wrote:
>>> Joel Fernandes <joelaf@google.com> writes:
>>>
>>>> Usage llist_del_first needs lock protection, however the table in the
>>>> comments of llist.h show a '-'. Correct this, and also add better
>>>> comments on top.
>>>>
>>>> Cc: Huang Ying <ying.huang@intel.com>
>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
>>>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>>>> ---
>>>>  include/linux/llist.h | 19 ++++++++++---------
>>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>>>> index fd4ca0b..15e4949 100644
>>>> --- a/include/linux/llist.h
>>>> +++ b/include/linux/llist.h
>>>> @@ -3,14 +3,15 @@
>>>>  /*
>>>>   * Lock-less NULL terminated single linked list
>>>>   *
>>>> - * If there are multiple producers and multiple consumers, llist_add
>>>> - * can be used in producers and llist_del_all can be used in
>>>> - * consumers.  They can work simultaneously without lock.  But
>>>> - * llist_del_first can not be used here.  Because llist_del_first
>>>> - * depends on list->first->next does not changed if list->first is not
>>>> - * changed during its operation, but llist_del_first, llist_add,
>>>> - * llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>>> - * another consumer may violate that.
>>>> + * If there are multiple producers and multiple consumers, llist_add can be
>>>> + * used in producers and llist_del_all can be used in consumers.  They can work
>>>> + * simultaneously without lock.  But llist_del_first will need to use a lock
>>>> + * with any other operation (ABA problem).  This is because llist_del_first
>>>> + * depends on list->first->next not changing but there's no way to be sure
>>>> + * about that and the cmpxchg in llist_del_first may succeed if list->first is
>>>> + * the same after concurrent operations. For example, a llist_del_first,
>>>> + * llist_add, llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>>> + * another consumer may cause violations.
>>>>   *
>>>>   * If there are multiple producers and one consumer, llist_add can be
>>>>   * used in producers and llist_del_all or llist_del_first can be used
>>>> @@ -19,7 +20,7 @@
>>>>   * This can be summarized as follow:
>>>>   *
>>>>   *           |   add    | del_first |  del_all
>>>> - * add       |    -     |     -     |     -
>>>> + * add       |    -     |     L     |     -
>>>
>>> If there are only one consumer which only calls llist_del_first(), lock
>>> is unnecessary.  So '-' is shown here originally.  But if there are
>>> multiple consumers which call llist_del_first() or llist_del_all(), lock
>>> is needed.
>>
>> I think this needs to be made more clear in the table. The table
>> doesn't clear say whether it describes the preceding paragraph
>> (multiple producers and one consumer), or if it describes the multiple
>> producers and one consumer case. So either we should have 2 tables, or
>
> Sorry, I meant "or if it describes the multiple producer and multiple
> consumer case".

I tried to describe both cases in the original table.

  *           |   add    | del_first |  del_all
  * add       |    -     |     -     |     -
  * del_first |          |     L     |     L
  * del_all   |          |           |     -

The 'L' for "del_first * del_first" means multiple consumers uses
llist_del_first() need lock.  And the 'L' for 'del_first * del_all'
means multiple consumers uses llist_del_first() and llist_del_all() need
lock.

Best Regards,
Huang, Ying

[toc] | [prev] | [next] | [standalone]


#1539014

FromJoel Fernandes <joelaf@google.com>
Date2016-12-09 03:30 +0100
Message-ID<sMj6G-5Kl-5@gated-at.bofh.it>
In reply to#1539011
On Thu, Dec 8, 2016 at 6:12 PM, Huang, Ying <ying.huang@intel.com> wrote:
> Joel Fernandes <joelaf@google.com> writes:
>
>> On Thu, Dec 8, 2016 at 4:42 PM, Joel Fernandes <joelaf@google.com> wrote:
>>> On Thu, Dec 8, 2016 at 4:35 PM, Huang, Ying <ying.huang@intel.com> wrote:
>>>> Joel Fernandes <joelaf@google.com> writes:
>>>>
>>>>> Usage llist_del_first needs lock protection, however the table in the
>>>>> comments of llist.h show a '-'. Correct this, and also add better
>>>>> comments on top.
>>>>>
>>>>> Cc: Huang Ying <ying.huang@intel.com>
>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
>>>>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>>>>> ---
>>>>>  include/linux/llist.h | 19 ++++++++++---------
>>>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>>>>> index fd4ca0b..15e4949 100644
>>>>> --- a/include/linux/llist.h
>>>>> +++ b/include/linux/llist.h
>>>>> @@ -3,14 +3,15 @@
>>>>>  /*
>>>>>   * Lock-less NULL terminated single linked list
>>>>>   *
>>>>> - * If there are multiple producers and multiple consumers, llist_add
>>>>> - * can be used in producers and llist_del_all can be used in
>>>>> - * consumers.  They can work simultaneously without lock.  But
>>>>> - * llist_del_first can not be used here.  Because llist_del_first
>>>>> - * depends on list->first->next does not changed if list->first is not
>>>>> - * changed during its operation, but llist_del_first, llist_add,
>>>>> - * llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>>>> - * another consumer may violate that.
>>>>> + * If there are multiple producers and multiple consumers, llist_add can be
>>>>> + * used in producers and llist_del_all can be used in consumers.  They can work
>>>>> + * simultaneously without lock.  But llist_del_first will need to use a lock
>>>>> + * with any other operation (ABA problem).  This is because llist_del_first
>>>>> + * depends on list->first->next not changing but there's no way to be sure
>>>>> + * about that and the cmpxchg in llist_del_first may succeed if list->first is
>>>>> + * the same after concurrent operations. For example, a llist_del_first,
>>>>> + * llist_add, llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>>>> + * another consumer may cause violations.
>>>>>   *
>>>>>   * If there are multiple producers and one consumer, llist_add can be
>>>>>   * used in producers and llist_del_all or llist_del_first can be used
>>>>> @@ -19,7 +20,7 @@
>>>>>   * This can be summarized as follow:
>>>>>   *
>>>>>   *           |   add    | del_first |  del_all
>>>>> - * add       |    -     |     -     |     -
>>>>> + * add       |    -     |     L     |     -
>>>>
>>>> If there are only one consumer which only calls llist_del_first(), lock
>>>> is unnecessary.  So '-' is shown here originally.  But if there are
>>>> multiple consumers which call llist_del_first() or llist_del_all(), lock
>>>> is needed.
>>>
>>> I think this needs to be made more clear in the table. The table
>>> doesn't clear say whether it describes the preceding paragraph
>>> (multiple producers and one consumer), or if it describes the multiple
>>> producers and one consumer case. So either we should have 2 tables, or
>>
>> Sorry, I meant "or if it describes the multiple producer and multiple
>> consumer case".
>
> I tried to describe both cases in the original table.
>
>   *           |   add    | del_first |  del_all
>   * add       |    -     |     -     |     -
>   * del_first |          |     L     |     L
>   * del_all   |          |           |     -
>
> The 'L' for "del_first * del_first" means multiple consumers uses
> llist_del_first() need lock.  And the 'L' for 'del_first * del_all'
> means multiple consumers uses llist_del_first() and llist_del_all() need
> lock.

Ok, now I get it - so basically the table describes one
producer/consumer vs another producer/consumer, in other words you are
just describing contention between any 2 operations. Thanks for
clarifying. I will respin the comments to explain this a bit better if
that's Ok with you.

Regards,
Joel

[toc] | [prev] | [next] | [standalone]


#1539015

From"Huang\, Ying" <ying.huang@intel.com>
Date2016-12-09 03:30 +0100
Message-ID<sMj6G-5Kl-13@gated-at.bofh.it>
In reply to#1539014
Joel Fernandes <joelaf@google.com> writes:

> On Thu, Dec 8, 2016 at 6:12 PM, Huang, Ying <ying.huang@intel.com> wrote:
>> Joel Fernandes <joelaf@google.com> writes:
>>
>>> On Thu, Dec 8, 2016 at 4:42 PM, Joel Fernandes <joelaf@google.com> wrote:
>>>> On Thu, Dec 8, 2016 at 4:35 PM, Huang, Ying <ying.huang@intel.com> wrote:
>>>>> Joel Fernandes <joelaf@google.com> writes:
>>>>>
>>>>>> Usage llist_del_first needs lock protection, however the table in the
>>>>>> comments of llist.h show a '-'. Correct this, and also add better
>>>>>> comments on top.
>>>>>>
>>>>>> Cc: Huang Ying <ying.huang@intel.com>
>>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
>>>>>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>>>>>> ---
>>>>>>  include/linux/llist.h | 19 ++++++++++---------
>>>>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>>>>>> index fd4ca0b..15e4949 100644
>>>>>> --- a/include/linux/llist.h
>>>>>> +++ b/include/linux/llist.h
>>>>>> @@ -3,14 +3,15 @@
>>>>>>  /*
>>>>>>   * Lock-less NULL terminated single linked list
>>>>>>   *
>>>>>> - * If there are multiple producers and multiple consumers, llist_add
>>>>>> - * can be used in producers and llist_del_all can be used in
>>>>>> - * consumers.  They can work simultaneously without lock.  But
>>>>>> - * llist_del_first can not be used here.  Because llist_del_first
>>>>>> - * depends on list->first->next does not changed if list->first is not
>>>>>> - * changed during its operation, but llist_del_first, llist_add,
>>>>>> - * llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>>>>> - * another consumer may violate that.
>>>>>> + * If there are multiple producers and multiple consumers, llist_add can be
>>>>>> + * used in producers and llist_del_all can be used in consumers.  They can work
>>>>>> + * simultaneously without lock.  But llist_del_first will need to use a lock
>>>>>> + * with any other operation (ABA problem).  This is because llist_del_first
>>>>>> + * depends on list->first->next not changing but there's no way to be sure
>>>>>> + * about that and the cmpxchg in llist_del_first may succeed if list->first is
>>>>>> + * the same after concurrent operations. For example, a llist_del_first,
>>>>>> + * llist_add, llist_add (or llist_del_all, llist_add, llist_add) sequence in
>>>>>> + * another consumer may cause violations.
>>>>>>   *
>>>>>>   * If there are multiple producers and one consumer, llist_add can be
>>>>>>   * used in producers and llist_del_all or llist_del_first can be used
>>>>>> @@ -19,7 +20,7 @@
>>>>>>   * This can be summarized as follow:
>>>>>>   *
>>>>>>   *           |   add    | del_first |  del_all
>>>>>> - * add       |    -     |     -     |     -
>>>>>> + * add       |    -     |     L     |     -
>>>>>
>>>>> If there are only one consumer which only calls llist_del_first(), lock
>>>>> is unnecessary.  So '-' is shown here originally.  But if there are
>>>>> multiple consumers which call llist_del_first() or llist_del_all(), lock
>>>>> is needed.
>>>>
>>>> I think this needs to be made more clear in the table. The table
>>>> doesn't clear say whether it describes the preceding paragraph
>>>> (multiple producers and one consumer), or if it describes the multiple
>>>> producers and one consumer case. So either we should have 2 tables, or
>>>
>>> Sorry, I meant "or if it describes the multiple producer and multiple
>>> consumer case".
>>
>> I tried to describe both cases in the original table.
>>
>>   *           |   add    | del_first |  del_all
>>   * add       |    -     |     -     |     -
>>   * del_first |          |     L     |     L
>>   * del_all   |          |           |     -
>>
>> The 'L' for "del_first * del_first" means multiple consumers uses
>> llist_del_first() need lock.  And the 'L' for 'del_first * del_all'
>> means multiple consumers uses llist_del_first() and llist_del_all() need
>> lock.
>
> Ok, now I get it - so basically the table describes one
> producer/consumer vs another producer/consumer, in other words you are
> just describing contention between any 2 operations. Thanks for
> clarifying. I will respin the comments to explain this a bit better if
> that's Ok with you.

It is good for me to improve the comments.

Best Regards,
Huang, Ying

[toc] | [prev] | [next] | [standalone]


#1538990

FromJoel Fernandes <joelaf@google.com>
Date2016-12-09 01:50 +0100
Message-ID<sMhxU-4EV-15@gated-at.bofh.it>
In reply to#1538987
On Thu, Dec 8, 2016 at 4:35 PM, Huang, Ying <ying.huang@intel.com> wrote:
> Joel Fernandes <joelaf@google.com> writes:
>
>> Usage llist_del_first needs lock protection, however the table in the
>> comments of llist.h show a '-'. Correct this, and also add better
>> comments on top.
>>
>> Cc: Huang Ying <ying.huang@intel.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
>> Signed-off-by: Joel Fernandes <joelaf@google.com>
>> ---
>>  include/linux/llist.h | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>> index fd4ca0b..15e4949 100644
>> --- a/include/linux/llist.h
>> +++ b/include/linux/llist.h
>> @@ -3,14 +3,15 @@
>>  /*
>>   * Lock-less NULL terminated single linked list
>>   *
>> - * If there are multiple producers and multiple consumers, llist_add
>> - * can be used in producers and llist_del_all can be used in
>> - * consumers.  They can work simultaneously without lock.  But
>> - * llist_del_first can not be used here.  Because llist_del_first
>> - * depends on list->first->next does not changed if list->first is not
>> - * changed during its operation, but llist_del_first, llist_add,
>> - * llist_add (or llist_del_all, llist_add, llist_add) sequence in
>> - * another consumer may violate that.
>> + * If there are multiple producers and multiple consumers, llist_add can be
>> + * used in producers and llist_del_all can be used in consumers.  They can work
>> + * simultaneously without lock.  But llist_del_first will need to use a lock
>> + * with any other operation (ABA problem).  This is because llist_del_first
>> + * depends on list->first->next not changing but there's no way to be sure
>> + * about that and the cmpxchg in llist_del_first may succeed if list->first is
>> + * the same after concurrent operations. For example, a llist_del_first,
>> + * llist_add, llist_add (or llist_del_all, llist_add, llist_add) sequence in
>> + * another consumer may cause violations.
>>   *
>>   * If there are multiple producers and one consumer, llist_add can be
>>   * used in producers and llist_del_all or llist_del_first can be used
>> @@ -19,7 +20,7 @@
>>   * This can be summarized as follow:
>>   *
>>   *           |   add    | del_first |  del_all
>> - * add       |    -     |     -     |     -
>> + * add       |    -     |     L     |     -
>
> If there are only one consumer which only calls llist_del_first(), lock
> is unnecessary.  So '-' is shown here originally.  But if there are
> multiple consumers which call llist_del_first() or llist_del_all(), lock
> is needed.

I think this needs to be made more clear in the table. The table
doesn't clear say whether it describes the preceding paragraph
(multiple producers and one consumer), or if it describes the multiple
producers and one consumer case. So either we should have 2 tables, or
just have one table for both cases and make it clear in the comments.
Like Instead of '-', say L* where * means locking is optional if
there's only one consumer.

Regards,
Joel

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web