Re: [PATCH -mm 08/25] add some sanity checks to get_scan_ratio

[Rik van Riel]

On Fri, 6 Jun 2008 18:04:47 -0700
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> > +	if (unlikely(zone->recent_scanned_file > file / 4)) {
> 
> I see nothing in the changelog about this and there are no comments. 
> How can a reader possibly work out what you were thinking when this
> was typed in??

Pulled into the main split LRU patch and commented.
 
> Perhaps the (nr_swap_pages <= 0) test could happen earlier on.

Done.
 
> Please quadruple-check this code like a paranoid maniac looking for
> underflows, overflows and divides-by-zero.  Bear in mind that x/(y+1)
> can get a div-by-zero for sufficiently-unepected values of y.

Done that already.

> Oh, so that's what the [0] and [1] in get_scan_ratio() mean.  Perhaps
> doing this:
> 
> 	if (nr_swap_pages <= 0) {
> 		percent[0] = 0;		/* anon */
> 		percent[1] = 100;	/* file */
> 
> would clarify things. 

Added lots of comments on this.

> > +++ linux-2.6.26-rc2-mm1/include/linux/mmzone.h	2008-05-28 12:11:51.000000000 -0400
> > @@ -289,6 +289,8 @@ struct zone {
> >  
> >  	unsigned long		recent_rotated_anon;
> >  	unsigned long		recent_rotated_file;
> > +	unsigned long		recent_scanned_anon;
> > +	unsigned long		recent_scanned_file;
> 
> I think struct zone is sufficiently important and obscure that
> field-by-field /*documentation*/ is needed.  Not as kerneldoc, please -
> better to do it at the definition site

Added documentation for these.

-- 
All rights reversed.
--
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/