ACCESS_ONCE usage inside llist_add_batch function
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 ?
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.
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.
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; }
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@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
participants (4)
-
Arun KS -
Chinmay V S -
Cihangir Akturk -
John de la Garza