Re: [PATCH RFC] struct list_node
On Sun, 10 Jun 2007 15:11:30 +1000, Rusty Russell wrote: > > +/* A list node is the same as the head of the list, but it's useful to > + * think of them as a separate type. */ > +struct list_node { > + struct list_head h; > +}; > + > +/* This allows us to support old style list_head as well as list_node. */ > +union list_head_or_node { > + struct list_head *_h; > + struct list_node *_n; > +}; > +union list_head_or_node_const { > + struct list_head *_h; > + struct list_node *_n; > + const struct list_head *_ch; > + const struct list_node *_cn; > +}; > +#define __lh(n) (((union list_head_or_node)(n))._h) > +#define __clh(n) (((union list_head_or_node_const)(n))._h) > + Any reason why you don't use __attribute__ ((transparent_union)) ? I guess this is exactly the case for which it was invented. Cheers, Jan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] struct list_node
On Sun, 2007-06-10 at 13:19 -0700, Linus Torvalds wrote: > And if you want a head, you really do want to use "hlist", since the head > is smaller than a list entry (a single pointer rather than two). No, now you're entirely missing the point. The normal Linux lists are beautiful, and should be used! I share your enthusiasm for them. They could just use a little more type safety for the common case. That is all. Hope that clarifies, Rusty. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] struct list_node
On Sun, 2007-06-10 at 10:20 -0700, Linus Torvalds wrote: > > On Sun, 10 Jun 2007, Rusty Russell wrote: > > > > The current list.h has the same type for list elements and list heads > > even though most code and coders treat them as distinct. > > I think the old list.h is technically superior to yours. > > Exactly *because* nodes and heads are interchangeable. > > In fact, you are incorrect that "most code" treat them as distinct. Most > code that uses list.h in fact uses it as a list of entries, often without > any head at all (and each *entry* is a point of removal), because the way > to actually *find* the structure that contains the lists is separate from > the lists themselves. Sorry, do you really believe a ring is "often" the case? It's not entirely trivial to audit, but I can do a random sample of 100 list_heads. The list iterators reinforce the "standalone head" model (ie. it's not called list_for_each_other_entry), and they're pretty popular. > The Linux kernel list.h is _better_ than most stupid list implementations > that think that a head node is different from the list node. Exactly > because it very naturally supports the notion of "this structure exists in > a 'ring of entries'" where each node is 100% equivalent to any other node, > and there _is_ no head. I agree it's a wonderful feature, but you talk about "no head" but they're all called "list_head"? In my version, they'd all be list_node, and you can trivially treat it as a head with ->h. It's a little more work for rings, but it's more explicit and gives us type checking on the common case. Thanks for your consideration, Rusty. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] struct list_node
On Sun, 10 Jun 2007, Linus Torvalds wrote: > > The Linux kernel list.h is _better_ than most stupid list implementations > that think that a head node is different from the list node. Exactly > because it very naturally supports the notion of "this structure exists in > a 'ring of entries'" where each node is 100% equivalent to any other node, > and there _is_ no head. Btw, to extend a bit on this: there actually *is* a "list with a head" implementation in , called "hlist". Now, the "h" actually historically stands for "hash", but if you prefer, you can think of it as standing for "head", and be happy. And if you want a head, you really do want to use "hlist", since the head is smaller than a list entry (a single pointer rather than two). And yes, I'm sure we could change some "struct list" users to "struct hlist" if you wanted to. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] struct list_node
On Sun, 10 Jun 2007, Rusty Russell wrote: > > The current list.h has the same type for list elements and list heads > even though most code and coders treat them as distinct. I think the old list.h is technically superior to yours. Exactly *because* nodes and heads are interchangeable. In fact, you are incorrect that "most code" treat them as distinct. Most code that uses list.h in fact uses it as a list of entries, often without any head at all (and each *entry* is a point of removal), because the way to actually *find* the structure that contains the lists is separate from the lists themselves. In other words, I think your patch is HORRIBLY BAD, because it totally obscures the beauty of the current list.h implementation, and makes it be something *average*. The Linux kernel list.h is _better_ than most stupid list implementations that think that a head node is different from the list node. Exactly because it very naturally supports the notion of "this structure exists in a 'ring of entries'" where each node is 100% equivalent to any other node, and there _is_ no head. And your patch totally misunderstands that, and breaks it. Nack. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] struct list_node
On Sun, 10 Jun 2007 15:11:30 +1000 Rusty Russell wrote: > The current list.h has the same type for list elements and list heads > even though most code and coders treat them as distinct. > > I've had a version of list.h (for userspace work) for about a year > which uses a different type for nodes and it works very well: code is > clearer, and mistakes like list_add() argument reversal are detected. > Code which really wants to treat a list node as a head can append ".h". > > To avoid a massive flag day, this patch uses gcc's "cast to union" to > allow either list_head or list_node in various places. > > Notes: > 1) A new function in_list() is introduced, equivalent to "list_empty(&e)" >but for nodes. in_list() sounds like it would scan an entire list and return true if &e is found, false if &e is not found... and that's what the short description sounds like to me as well... I'm just confuzed. And you aren't supposed to write confuzing interfaces. :) > +/** > + * in_list - tests whether element is in a list. > + * @entry: the entry to test > + * > + * Returns false if the list elem was deleted from list (except __list_del) What is "elem"? How can this function determine is a list element was deleted vs. was never added? > + */ > +static inline int in_list(const struct list_node *entry) > +{ > + return entry->h.next == &entry->h; > } --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH RFC] struct list_node
The current list.h has the same type for list elements and list heads even though most code and coders treat them as distinct. I've had a version of list.h (for userspace work) for about a year which uses a different type for nodes and it works very well: code is clearer, and mistakes like list_add() argument reversal are detected. Code which really wants to treat a list node as a head can append ".h". To avoid a massive flag day, this patch uses gcc's "cast to union" to allow either list_head or list_node in various places. Notes: 1) A new function in_list() is introduced, equivalent to "list_empty(&e)" but for nodes. 2) The duplicated kerneldoc in list_debug.c is excised. 3) The more obscure list functions have yet to be converted. Signed-off-by: Rusty Russell <[EMAIL PROTECTED]> diff -r bea2b8147985 include/linux/list.h --- a/include/linux/list.h Thu Jun 07 22:50:36 2007 +1000 +++ b/include/linux/list.h Sun Jun 10 14:56:56 2007 +1000 @@ -22,6 +22,26 @@ struct list_head { struct list_head *next, *prev; }; +/* A list node is the same as the head of the list, but it's useful to + * think of them as a separate type. */ +struct list_node { + struct list_head h; +}; + +/* This allows us to support old style list_head as well as list_node. */ +union list_head_or_node { + struct list_head *_h; + struct list_node *_n; +}; +union list_head_or_node_const { + struct list_head *_h; + struct list_node *_n; + const struct list_head *_ch; + const struct list_node *_cn; +}; +#define __lh(n) (((union list_head_or_node)(n))._h) +#define __clh(n) (((union list_head_or_node_const)(n))._h) + #define LIST_HEAD_INIT(name) { &(name), &(name) } #define LIST_HEAD(name) \ @@ -63,15 +83,15 @@ extern void __list_add(struct list_head * Insert a new entry after the specified head. * This is good for implementing stacks. */ +#define list_add(new, head) list_add_lh(__lh(new), head) #ifndef CONFIG_DEBUG_LIST -static inline void list_add(struct list_head *new, struct list_head *head) +static inline void list_add_lh(struct list_head *new, struct list_head *head) { __list_add(new, head, head->next); } #else -extern void list_add(struct list_head *new, struct list_head *head); +extern void list_add_lh(struct list_head *new, struct list_head *head); #endif - /** * list_add_tail - add a new entry @@ -81,7 +101,9 @@ extern void list_add(struct list_head *n * Insert a new entry before the specified head. * This is useful for implementing queues. */ -static inline void list_add_tail(struct list_head *new, struct list_head *head) +#define list_add_tail(new, head) list_add_tail_lh(__lh(new), head) +static inline void list_add_tail_lh(struct list_head *new, + struct list_head *head) { __list_add(new, head->prev, head); } @@ -118,7 +140,9 @@ static inline void __list_add_rcu(struct * the _rcu list-traversal primitives, such as * list_for_each_entry_rcu(). */ -static inline void list_add_rcu(struct list_head *new, struct list_head *head) +#define list_add_rcu(new, head) list_add_rcu_lh(__lh(new), (head)) +static inline void list_add_rcu_lh(struct list_head *new, + struct list_head *head) { __list_add_rcu(new, head, head->next); } @@ -139,7 +163,8 @@ static inline void list_add_rcu(struct l * the _rcu list-traversal primitives, such as * list_for_each_entry_rcu(). */ -static inline void list_add_tail_rcu(struct list_head *new, +#define list_add_tail_rcu(new, head) list_add_tail_rcu_lh(__lh(new), (head)) +static inline void list_add_tail_rcu_lh(struct list_head *new, struct list_head *head) { __list_add_rcu(new, head->prev, head); @@ -164,15 +189,16 @@ static inline void __list_del(struct lis * Note: list_empty() on entry does not return true after this, the entry is * in an undefined state. */ +#define list_del(entry) list_del_lh(__lh(entry)) #ifndef CONFIG_DEBUG_LIST -static inline void list_del(struct list_head *entry) +static inline void list_del_lh(struct list_head *entry) { __list_del(entry->prev, entry->next); entry->next = LIST_POISON1; entry->prev = LIST_POISON2; } #else -extern void list_del(struct list_head *entry); +extern void list_del_lh(struct list_head *entry); #endif /** @@ -199,7 +225,8 @@ extern void list_del(struct list_head *e * or call_rcu() must be used to defer freeing until an RCU * grace period has elapsed. */ -static inline void list_del_rcu(struct list_head *entry) +#define list_del_rcu(e) list_del_rcu_lh(__lh(e)) +static inline void list_del_rcu_lh(struct list_head *entry) { __list_del(entry->prev, entry->next); entry->prev = LIST_POISON2; @@ -251,7 +278,8 @@ static inline void list_replace_rcu(stru * list_del_init - deletes entry from list and reinitialize it. * @entry: the element to delete from the list. */