Hi, Mathieu,
Thanks for review.
On 04/08/2011 02:30 AM, Mathieu Desnoyers wrote:
> * Huang Ying (ying.huang@xxxxxxxxx) wrote:
[snip]
>> +/**
>> + * llist_for_each - iterate over some deleted entries of a lock-less list
>> + * @pos: the &struct llist_node to use as a loop cursor
>> + * @node: the first entry of deleted list entries
>> + *
>> + * In general, some entries of the lock-less list can be traversed
>> + * safely only after being deleted from list, so start with an entry
>> + * instead of list head.
>> + *
>> + * If being used on entries deleted from lock-less list directly, the
>> + * traverse order is from the newest to the oldest added entry. If
>> + * you want to traverse from the oldest to the newest, you must
>> + * reverse the order by yourself before traversing.
>> + */
>> +#define llist_for_each(pos, node) \
>> + for (pos = (node); pos; pos = pos->next)
>
> I know list.h has the same lack of ( ) around "pos" in the for_each
> iterator, but shouldn't we add some around it to ensure that especially
> (pos)->next uses the right operator prececence ? e.g.
>
> for ((pos) = (node); pos; (pos) = (pos)->next)
>
> maybe there is some reason for not putting parenthesis there that I am
> missing, but I'm asking anyway.
Don't know either. But I think there should be no harm to add
parenthesis here. Will change this and similar code in patch.
[snip]
>> +/**
>> + * llist_empty - tests whether a lock-less list is empty
>> + * @head: the list to test
>> + *
>> + * Not guaranteed to be accurate or up to date. Just a quick way to
>> + * test whether the list is empty without deleting something from the
>> + * list.
>> + */
>> +static inline int llist_empty(const struct llist_head *head)
>> +{
>> + return head->first == NULL;
>
> Would it make sense to do:
>
> return ACCESS_ONCE(head->first) == NULL;
>
> instead ? Otherwise the compiler can choose to keep the result around in
> registers without re-reading (e.g. busy waiting loop).
Although I think that llist_empty() in a loop is not the typical usage
model, adding ACCESS_ONCE can support that better without other harm. I
will change this.
[snip]
>> + * The basic atomic operation of this list is cmpxchg on long. On
>> + * architectures that don't have NMI-safe cmpxchg implementation, the
>> + * list can NOT be used in NMI handler. So code uses the list in NMI
>> + * handler should depend on CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG.
>> + *
>> + * Copyright 2010 Intel Corp.
>
> 2010, 2011
Will change this.
[snip]
>> +/**
>> + * llist_add - add a new entry
>> + * @new: new entry to be added
>> + * @head: the head for your lock-less list
>> + */
>> +void llist_add(struct llist_node *new, struct llist_head *head)
>> +{
>> + struct llist_node *entry;
>> +
>> +#ifndef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
>> + BUG_ON(in_nmi());
>> +#endif
>> +
>> + do {
>> + entry = head->first;
>> + new->next = entry;
>> + cpu_relax();
>> + } while (cmpxchg(&head->first, entry, new) != entry);
>
> Could be turned into:
>
> struct llist_node *entry, *old_entry;
>
> entry = head->first;
>
> do {
> old_entry = entry;
> new->next = entry;
> cpu_relax();
> } while ((entry = cmpxchg(&head->first, old_entry, new)) != old_entry);
>
> It should generate more compact code, and slightly faster retry.
Yes. Will change this and similar code in patch.
Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
