Re: [RFC] llist: Fix code comments about llist_del_first locking

From: Joel Fernandes <joelaf@google.com>
To: "Huang, Ying" <ying.huang@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>, Will Deacon <will.deacon@arm.com>,
	Paul McKenney <paulmck@linux.vnet.ibm.com>
Subject: Re: [RFC] llist: Fix code comments about llist_del_first locking
Date: Thu, 8 Dec 2016 16:42:29 -0800	[thread overview]
Message-ID: <CAJWu+orc2P19U__foZ1yHxuuq-0rgg-MkPX+SNr4_DgjLvk8dw@mail.gmail.com> (raw)
In-Reply-To: <878trq3pnd.fsf@yhuang-dev.intel.com>

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