To: Christoph Hellwig <hch@infradead.org>, David Howells <dhowells@redhat.com>,
       torvalds@transmeta.com, linux-kernel@vger.kernel.org,
       linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] PAG support, try #2 
In-Reply-To: <20030514115653.A15202@infradead.org> 
User-Agent: EMH/1.14.1 SEMI/1.14.4 (Hosorogi) FLIM/1.14.4
 (=?ISO-8859-4?Q?Kashiharajing=FE-mae?=) APEL/10.4 Emacs/21.2
 (i386-redhat-linux-gnu) MULE/5.0 (SAKAKI)
MIME-Version: 1.0 (generated by SEMI 1.14.4 - "Hosorogi")
Content-Type: text/plain; charset=US-ASCII
Date: Wed, 14 May 2003 12:56:04 +0100
Message-ID: <30949.1052913364@warthog.warthog>
From: David Howells <dhowells@warthog.cambridge.redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2167
Lines: 64


> These have to be a marked asmlinkage.

Damn. Fixed.

> What's the reason you have the syscall in this header anyway?

So they can be called from the AFS stub. However, the sys_*() versions
shouldn't be there.

> > +static kmem_cache_t *vfs_token_cache;
> > +static kmem_cache_t *vfs_pag_cache;
> 
> So do you have an estimate for the number of users here yet?
> Adding two more slab caches that are unused for 99% of the users
> might not be the best choice if there's no strong need.

There won't be many PAGs. Basically one per login session would be fairly
typical, and possibly one per SUID program run at some later date.

> > +inline pag_t vfs_leave_pag(void)
> 
> Inline but not static seems strange..

It's not without precedent within the kernel. The compiler is free to inline
it, but must also emit an out-of-line copy. Thinking about it, it's probably
not worth it... these calls aren't going to be called very often and so don't
need to be optimised for every last ounce of speed.

> We already discussed the coding style issue,

Well, the coding style is wrong here IMNSHO. Readability is preferable.

> but anyway, why aren't these three separate syscalls?

I'm trying not to cause hyper syscall breeding, but since they are three
separate logical functions, it I'll split it out anyway.

> What protects vfspag->tokens?

Why does it need to be protected at that point? The PAG no longer has any
references, and the tokens don't point back to it, and are in any case pinned
by virtue of being on the list.

> Please linwrap after 80 chars.

Done.

> Shouldn't vfs_pag_get hanlde a NULL argument instead?

Maybe. But then that's introducing a conditional branch that you can't avoid,
even if you know it's going to succeed:-/

> What happened to the suggestion to make the pag code optional?  It's
> really easy to stub it out properly and most people don't need it.

Okay, I've conditionalised it.

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