ACCESS_ONCE usage inside llist_add_batch function

ACCESS_ONCE usage inside llist_add_batch function

[Cihangir Akturk]

Reading the lib/llist.c file in the kernel sources, I came across
the llist_add_bach function defined like this;

bool llist_add_batch(struct llist_node *new_first, struct llist_node *new_last,
		     struct llist_head *head)
{
	struct llist_node *first;

	do {
		new_last->next = first = ACCESS_ONCE(head->first);
	} while (cmpxchg(&head->first, first, new_first) != first);

	return !first;
}

One thing bugging my mind is the ACCESS_ONCE macro. Is it really
needed here ? I mean I would write this function with ACCES_ONCE
moved outside the loop like as follows;

bool llist_add_batch(struct llist_node *new_first, struct llist_node *new_last,
		     struct llist_head *head)
{
	struct llist_node *first, *old;

	old = ACCESS_ONCE(head->first);
	for (;;) {
		first = old;
		new_last->next = old;
		old = cmpxchg(&head->first, first, new_first);
		if (old == first)
			break;
	}

	return !first;
}

I think that it may be faster to just use the return value of cmpxchg.
But I am not sure about this.

Is my understanding correct ?

ACCESS_ONCE usage inside llist_add_batch function

[John de la Garza]

On Sat, Feb 28, 2015 at 10:12:23PM +0200, Cihangir Akturk wrote:
> Reading the lib/llist.c file in the kernel sources, I came across
> the llist_add_bach function defined like this;
> 
> bool llist_add_batch(struct llist_node *new_first, struct llist_node *new_last,
> 		     struct llist_head *head)
> {
> 	struct llist_node *first;
> 
> 	do {
> 		new_last->next = first = ACCESS_ONCE(head->first);
> 	} while (cmpxchg(&head->first, first, new_first) != first);
> 
> 	return !first;
> }
> 
> One thing bugging my mind is the ACCESS_ONCE macro. Is it really
> needed here ? I mean I would write this function with ACCES_ONCE
> moved outside the loop like as follows;
> 
Do you know what ACCESS_ONCE does?

I found this comment:

"The compiler
 * is also forbidden from reordering successive instances of ACCESS_ONCE()"

from: include/linux/compiler.h


This might be something that does need to be called (to tell the compiler
something) on every loop iteration.

ACCESS_ONCE usage inside llist_add_batch function

[Chinmay V S]

On Tue, Mar 3, 2015 at 11:51 AM, John de la Garza <john@jjdev.com> wrote:
> On Sat, Feb 28, 2015 at 10:12:23PM +0200, Cihangir Akturk wrote:
>> Reading the lib/llist.c file in the kernel sources, I came across
>> the llist_add_bach function defined like this;
>>
>> bool llist_add_batch(struct llist_node *new_first, struct llist_node *new_last,
>>                    struct llist_head *head)
>> {
>>       struct llist_node *first;
>>
>>       do {
>>               new_last->next = first = ACCESS_ONCE(head->first);
>>       } while (cmpxchg(&head->first, first, new_first) != first);
>>
>>       return !first;
>> }
>>
>> One thing bugging my mind is the ACCESS_ONCE macro. Is it really
>> needed here ? I mean I would write this function with ACCES_ONCE
>> moved outside the loop like as follows;

Replace ACCESS_ONCE() with volatile and you would most likely
understand that it is not what it looks like.
A very unfortunate consequence of what the macro is named.
A better name probably would have been - REALLY_REALLY_ACCESS_ONCE()

Checkout http://lwn.net/Articles/624126/ for some obscure bugs and
future plans for this.

ACCESS_ONCE usage inside llist_add_batch function

[Cihangir Akturk]

On Tue, Mar 03, 2015 at 12:08:41PM +0530, Chinmay V S wrote:
> On Tue, Mar 3, 2015 at 11:51 AM, John de la Garza <john@jjdev.com> wrote:
> > On Sat, Feb 28, 2015 at 10:12:23PM +0200, Cihangir Akturk wrote:
> >> Reading the lib/llist.c file in the kernel sources, I came across
> >> the llist_add_bach function defined like this;
> >>
> >> bool llist_add_batch(struct llist_node *new_first, struct llist_node *new_last,
> >>                    struct llist_head *head)
> >> {
> >>       struct llist_node *first;
> >>
> >>       do {
> >>               new_last->next = first = ACCESS_ONCE(head->first);
> >>       } while (cmpxchg(&head->first, first, new_first) != first);
> >>
> >>       return !first;
> >> }
> >>
> >> One thing bugging my mind is the ACCESS_ONCE macro. Is it really
> >> needed here ? I mean I would write this function with ACCES_ONCE
> >> moved outside the loop like as follows;
> 
> Replace ACCESS_ONCE() with volatile and you would most likely
> understand that it is not what it looks like.
> A very unfortunate consequence of what the macro is named.
> A better name probably would have been - REALLY_REALLY_ACCESS_ONCE()

Thanks for the reply but this isn't the question I asked.

> Checkout http://lwn.net/Articles/624126/ for some obscure bugs and
> future plans for this.

I didn't know the non-scalar type related issue and there is a
READ_ONCE macro out there but again there is nothing to do with
my question because as you can see there isn't any non-scalar
access being done in the code.

So my question is this; do we really need to use 
ACCESS_ONCE(head->first) on every iteration instead of just using the
return value of cmpxchg function like as follows ? [1]

[1] sample code
struct llist_node *first, *old_first;

old_first = ACCESS_ONCE(head->first);
for (;;) {
	first = old_first;
	new_last->next = old_first;
	old_first = cmpxchg(&head->first, first, new_first);
	if (old_first == first)
		break;
}

ACCESS_ONCE usage inside llist_add_batch function

[Arun KS]

Hi Akturk,

On Sun, Mar 1, 2015 at 1:42 AM, Cihangir Akturk <cakturk@gmail.com> wrote:

> Reading the lib/llist.c file in the kernel sources, I came across
> the llist_add_bach function defined like this;
>
> bool llist_add_batch(struct llist_node *new_first, struct llist_node
> *new_last,
>                      struct llist_head *head)
> {
>         struct llist_node *first;
>
>         do {
>                 new_last->next = first = ACCESS_ONCE(head->first);
>         } while (cmpxchg(&head->first, first, new_first) != first);
>
>         return !first;
> }
>
> One thing bugging my mind is the ACCESS_ONCE macro. Is it really
> needed here ? I mean I would write this function with ACCES_ONCE
> moved outside the loop like as follows;
>

ACCESS_ONCE avoids compiler optimization. A kind of compiler optimization
can be caching a value in a register and reusing it.
Why compiler does this? Its his job to create optimized code for speed by
default.

Now llist is lock less linked list, where multiple consumers and producers
can work simultaneously.
Hence during each iteration of the while loop, you want to access the
current value of head->first.

And if you remove ACCESS_ONCE, compiler can keep a copy of head->list in a
register during first iteration and reuse it from register for the
subsequent iterations.

Thanks,
Arun


>
> bool llist_add_batch(struct llist_node *new_first, struct llist_node
> *new_last,
>                      struct llist_head *head)
> {
>         struct llist_node *first, *old;
>
>         old = ACCESS_ONCE(head->first);
>         for (;;) {
>                 first = old;
>                 new_last->next = old;
>                 old = cmpxchg(&head->first, first, new_first);
>                 if (old == first)
>                         break;
>         }
>
>         return !first;
> }
>
> I think that it may be faster to just use the return value of cmpxchg.
> But I am not sure about this.
>
> Is my understanding correct ?
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150304/5d78b097/attachment.html