[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 10 Nov 2008 21:04:05 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: Kentaro Takeda <takedakn@...data.co.jp>
Cc: haradats@...data.co.jp, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, penguin-kernel@...ove.SAKURA.ne.jp
Subject: Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname
management functions.
On Mon, 10 Nov 2008 19:34:17 +0900 Kentaro Takeda <takedakn@...data.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists