Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname management functions.

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 10 Nov 2008 19:34:17 +0900 Kentaro Takeda <takedakn@nttdata.co.jp> wrote:

> 
> ...
>
> > > +/**
> > > + * tmy_alloc - Allocate memory for temporal purpose.
> > > + *
> > > + * @size: Size in bytes.
> > > + *
> > > + * Returns pointer to allocated memory on success, NULL otherwise.
> > > + */
> > > +void *tmy_alloc(const size_t size)
> > > +{
> > > +	void *p = kzalloc(size, GFP_KERNEL);
> > > +	if (p)
> > > +		atomic_add(ksize(p), &dynamic_memory_size);
> > > +	return p;
> > > +}
> > 
> > Note that I said "kmalloc", not "kzalloc".  This function zeroes
> > everything all the time, and surely that is not necessary.  It's just a
> > waste of CPU time.
> > 
> Callers of tmy_alloc assume that allocated memory is zeroed.

That isn't the point.  For programmer convenience we could make
__alloc_pages() and kmalloc() zero all the memory too.  But we don't
because it is slow.

> > > +/**
> > > + * tmy_read_memory_counter - Check for memory usage.
> > > + *
> > > + * @head: Pointer to "struct tmy_io_buffer".
> > > + *
> > > + * Returns memory usage.
> > 
> > In what units?  Megabytes?
> > 
> In bytes.

Let me rephrase:

The comment over tmy_read_memory_counter() fails to tell the reader
what units are used for the return value.  It should do so.

> > Again, we would like to see a complete decription of the proposed
> > userspace ABI.  This one looks fairly ugly.  Do I really have to write
> > 'S' 'h' 'a' 'r' 'e' 'd' ':' ' ' into some pseudo file?
> > 
> > A better interface would be two suitably-named pseudo files each of
> > which takes a bare integer string.  None of this funny colon-based
> > prefixing stuff.
> > 
> Creating pseudo files for each variables is fine, though I don't see
> advantage by changing from
> "echo Shared: 16777216 > /sys/kernel/security/tomoyo/meminfo" to
> "echo 16777216 > /sys/kernel/security/tomoyo/quota/shared_memory".

Well for starters, the existing interface is ugly as sin and will make
kernel developers unhappy.

There is a pretty strict one-value-per-file rule in sysfs files, and
"multiple tagged values in one file" violates that a lot.