On Mon, Jul 31, 2000 at 11:42:06PM +0200, Linus Torvalds wrote:
> > Anyways, hlists are already used all over the kernel (e.g. try grep pprev
> > net/ipv4/*), just everybody is reinventing the wheel on them all the
> > time. I did that myself several times. It would be nicer to use list_*()
> > macros the time, just without the bloat of the list_* list heads.
>
> Noe that THIS is a valid argument that I can find no holes in.
>
> The argument of "inode.c could be speeded up/shrunk/xxxx" doesn't strike
> me as being a very good argument especially just before 2.4.x.
Ok, sorry for forgetting about the Quake monster @)
>
> The argument that "lots of code already does this, except they aren't very
> clean about it and do it by hand", is an argument I can buy into.
>
> You might consider just going about it a different way: pick the places
> that _already_ use this kind of list, and clean them up using a generic
> list package. I still don't like "hlists" as a name, because I still don't
> see the "hash" in them conceptually, but I would certainyl consider any
> cleanup a good thing.
I don't particularly like the name neither, I just couldn't think of a better
one.
>
> And once you come from that direction, it's going to be a lot easier
> convincing me to eventually potentially switch over some of the current
> lists.h users to a new implementation.
Ok, I'll do in 2.5.
I extracted the pure bugfix parts of my patch for 2.4. dcache.c uses
list_empty() several times to check if a dentry is unhashed or not.
This requires a INIT_LIST_HEAD after the list_del. Unfortunately some users
forget the INIT_LIST_HEAD, which probably could lead to races.
I added a new inline list_del_init() and fixed the few misusers (and actually
converted a few who use INIT_LIST_HEAD too to make it look nicer).
Please apply the patch (appended)
>
> > I suspect that would either end up with lots of pseudo functional function
> > pointer (do_foo(hash_list, void (*foo_functor)(void *, void *)) and other slow
> > horrors) or a disgusting macro mess like the older lists.h that was
> > recently removed.
>
> I'm not convinced. The wnew list.h in my opinion does really well, and
> _without_ having a lot of ugly macros. It's strange to people using the
> BSD ones, but it has, in my opinion, a much cleaner interface. I think the
> same approach could be extended to cover the needs of a nice hash table.
>
> For example, the current lists have the list_for_each() thing that walks
> the list. The "hash_find(list)" thing wouldn't need to be all that
> different from that one - let people supply their own hash comparison
> functions etc not by giving them as arguments, but by creating nice
> constructs together with the helper functions.
Sounds like a good way to justify statement expressions ;)
I've seen similar constructs in perl though and didn't like it too much there.
I'll have to think about it.
Just even with higher level hash table support the low level primitives
like _del, _add etc. would still be needed, and they look very similar
to list_*.
-Andi
--- fs/autofs4/root.c-LFIX Wed Jul 5 20:31:01 2000
+++ fs/autofs4/root.c Tue Aug 1 03:04:05 2000
@@ -400,7 +400,7 @@
spin_unlock(&dcache_lock);
return -ENOTEMPTY;
}
- list_del(&dentry->d_hash);
+ list_del_init(&dentry->d_hash);
spin_unlock(&dcache_lock);
dput(ino->dentry);
--- fs/dcache.c-LFIX Fri Jul 28 01:47:16 2000
+++ fs/dcache.c Tue Aug 1 03:07:31 2000
@@ -80,8 +80,7 @@
struct inode *inode = dentry->d_inode;
if (inode) {
dentry->d_inode = NULL;
- list_del(&dentry->d_alias);
- INIT_LIST_HEAD(&dentry->d_alias);
+ list_del_init(&dentry->d_alias);
spin_unlock(&dcache_lock);
if (dentry->d_op && dentry->d_op->d_iput)
dentry->d_op->d_iput(dentry, inode);
@@ -152,7 +151,7 @@
return;
unhash_it:
- list_del(&dentry->d_hash);
+ list_del_init(&dentry->d_hash);
kill_it: {
struct dentry *parent;
@@ -217,8 +216,7 @@
}
}
- list_del(&dentry->d_hash);
- INIT_LIST_HEAD(&dentry->d_hash);
+ list_del_init(&dentry->d_hash);
spin_unlock(&dcache_lock);
return 0;
}
@@ -306,7 +304,7 @@
{
struct dentry * parent;
- list_del(&dentry->d_hash);
+ list_del_init(&dentry->d_hash);
list_del(&dentry->d_child);
dentry_iput(dentry);
parent = dentry->d_parent;
@@ -341,8 +339,7 @@
if (tmp == &dentry_unused)
break;
dentry_stat.nr_unused--;
- list_del(tmp);
- INIT_LIST_HEAD(tmp);
+ list_del_init(tmp);
dentry = list_entry(tmp, struct dentry, d_lru);
/* Unused dentry with a count? */
--- include/linux/list.h-LFIX Mon Jun 19 22:42:43 2000
+++ include/linux/list.h Tue Aug 1 03:03:01 2000
@@ -85,10 +85,21 @@
/**
* list_del - deletes entry from list.
* @entry: the element to delete from the list.
+ * Note: list_empty on entry does not return true after this, the entry is in an undefined state.
*/
static __inline__ void list_del(struct list_head *entry)
{
__list_del(entry->prev, entry->next);
+}
+
+/**
+ * list_del_init - deletes entry from list and reinitialize it.
+ * @entry: the element to delete from the list.n
+ */
+static __inline__ void list_del_init(struct list_head *entry)
+{
+ __list_del(entry->prev, entry->next);
+ INIT_LIST_HEAD(entry);
}
/**
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Mon Jul 31 2000 - 21:00:35 EST