Re: [PATCH] Prezeroing V8

[Andrew Morton]

Christoph Lameter <clameter@xxxxxxx> wrote:
>
> On Thu, 17 Mar 2005, Andrew Morton wrote:
> 
> > Christoph Lameter <clameter@xxxxxxx> wrote:
> > >
> > > Adds management of ZEROED and NOT_ZEROED pages and a background daemon
> > > called scrubd. /proc/sys/vm/scrubd_load, /proc/sys/vm_scrubd_start and
> > > /proc/sys/vm_scrubd_stop control the scrub daemon. See Documentation/vm/
> > > scrubd.txt
> >
> > It's hard to know what to think about this without benchmarking numbers.

?

> >
> > It would help if you could briefly describe the implementation and design
> > decisions when sending patches.
> 
> Oh. This was discussed so many times that I thought it would not be
> necessary anymore. The discussion is attached.

Add it to the changelog and maintain it, please.  It never hurts.

But that only describes why we want the feature, which is nice.  It's also
useful to explain how the feature works.  Although my preference there is
that this be done within code comments if at all appropriate.

<looks>

OK, so we're splitting each zone's buddy structure into two: one for zeroed
pages and one for not-zeroed pages, yes?

It's not obvious what the page->private of freed pages are being used for. 
Please comment that.

What's all this (zero << 10) stuff?

+	page->private = order + (zero << 10);
+           (page_zorder(page) == order + (zero << 10)) &&

Doesn't this explode if we already have order-1024 pages in there?  I guess
that's a reasonable restriction, but where did the "10" come from? 
Non-obvious, needs commenting.

And given that we have separate buddy structures for zeroed and not-zeroed
pages, why is this tagging needed at all?


These are all design decisions which have been made, but they're not
communicated either in the patch description or in code comments.  It's to
everyone's advantage to fix that, no?

-
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/