Re: [PATCH 03/13] list: Add list_add_(before|after) macros

[Hugh Dickins <hughd@google.com>]

On Fri, 6 Jun 2014, Ken Helias wrote:
> From: Ken Helias <kenhelias@firemail.de>
> 
> Many places in the code uses list_add_tail/list_add to insert an entry
> before/after another entry. This confuses the reader because these are usually
> used to add an item to a list_head and not an entry. hlist already have
> functions to do the same which makes the code a lot more readable. It would be
> a lot easier to understand when all places use a self explaining function name
> instead of misusing other functions.

I'll express my opinion: perhaps it's widely shared, perhaps it is not.

I think a patch like this (and the subsequent conversions) is unhelpful
churn.

The Linux struct list_head is designed and intended to be used for both
head and entry, and introducing aliases to make unnecessary distinctions
just hinders development instead of helping it.

If the reader is confused at first, I hope the reader will soon learn,
without needing to rely on aliases such as these.

Hugh

> 
> Signed-off-by: Ken Helias <kenhelias@firemail.de>
> Cc: Dipankar Sarma <dipankar@in.ibm.com>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ken Helias <kenhelias@firemail.de>
> Cc: Dave Jones <davej@redhat.com>
> ---
>  include/linux/list.h    | 22 ++++++++++++++++++++++
>  include/linux/rculist.h | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/include/linux/list.h b/include/linux/list.h
> index ab43d01..5ea0eca 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -76,6 +76,28 @@ static inline void list_add_tail(struct list_head *new, struct list_head *head)
>  	__list_add(new, head->prev, head);
>  }
>  
> +/**
> + * list_add_after
> + * @n: new entry to be added
> + * @prev: the existing element to add the new element after.
> + *
> + * Description:
> + * Adds the specified element to the specified list
> + * after the specified node.
> + */
> +#define list_add_after(n, prev) list_add(n, prev)
> +
> +/**
> + * list_add_before
> + * @n: new entry to be added
> + * @next:  the existing element to add the new element before.
> + *
> + * Description:
> + * Adds the specified element to the specified list
> + * before the specified node.
> + */
> +#define list_add_before(n, next) list_add_tail(n, next)
> +
>  /*
>   * Delete a list entry by making the prev/next entries
>   * point to each other.
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index 648773f..eec274d 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -103,6 +103,44 @@ static inline void list_add_tail_rcu(struct list_head *new,
>  }
>  
>  /**
> + * list_add_after_rcu
> + * @n: new entry to be added
> + * @prev: the existing element to add the new element after.
> + *
> + * Description:
> + * Adds the specified element to the specified list
> + * after the specified node while permitting racing traversals.
> + *
> + * The caller must take whatever precautions are necessary
> + * (such as holding appropriate locks) to avoid racing
> + * with another list-mutation primitive, such as list_add_rcu()
> + * or list_del_rcu(), running on this same list.
> + * However, it is perfectly legal to run concurrently with
> + * the _rcu list-traversal primitives, such as
> + * list_for_each_entry_rcu().
> + */
> +#define list_add_after_rcu(n, next) list_add_rcu(n, next)
> +
> +/**
> + * list_add_before_rcu
> + * @n: new entry to be added
> + * @next:  the existing element to add the new element before.
> + *
> + * Description:
> + * Adds the specified element to the specified list
> + * before the specified node while permitting racing traversals.
> + *
> + * The caller must take whatever precautions are necessary
> + * (such as holding appropriate locks) to avoid racing
> + * with another list-mutation primitive, such as list_add_tail_rcu()
> + * or list_del_rcu(), running on this same list.
> + * However, it is perfectly legal to run concurrently with
> + * the _rcu list-traversal primitives, such as
> + * list_for_each_entry_rcu().
> + */
> +#define list_add_before_rcu(n, next) list_add_tail_rcu(n, next)
> +
> +/**
>   * list_del_rcu - deletes entry from list without re-initialization
>   * @entry: the element to delete from the list.
>   *
> -- 
> 2.0.0