Re: [patch] epoll use a single inode ...

[Linus Torvalds]

On Wed, 7 Mar 2007, Eric Dumazet wrote:
> 
> OK no problem here is the patch without messing f_path.mnt 

All right. I like it. Definitely worth putting into -mm, or just 
re-sending to me after 2.6.21 is out (I'll forget all about it otherwise).

I have one more worry, namely this::

	-       char name[32];
	-       this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
	-       this.name = name;
	+       this.len = 0;
	+       this.name = NULL;

I think that's fine, and it *happens* to work, but it happens to work just 
because then d_alloc() will do:

	memcpy(dname, name->name, name->len);
	dname[name->len] = 0;

and passing in NULL to memcpy() is generally ok when len is 0.

HOWEVER. 

Not only might memcpy() do a "prefetch for read" on the source for some 
architectures (which in turn may end up being slow for an address that 
isn't in the TLB, like NULL), but you depend on a very much internal 
detail, since it *could* have been using something like

	memcpy(dname, name->name, name->len+1);

instead, and expected to get the final '\0' character from the source 
string.

So I would actually much prefer it to be written as

	this.len = 0;
	this.name = "";

just because it's safer.

But other than that small detail, I think this is not only an 
optimization, it's an actual cleanup, and we migth some day want to use 
something like this for some other things too (eg maybe this kind of 
approach is usable for /proc/<pid> entries too, to avoid instantiating 
them).

As to avoiding the mntget(), I'm not violently opposed to it, but I do 
think that it's a totally unrelated matter, so even if it's decided it's 
worth it, it should be done as a separate patch regardless.

			Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/