[PATCH] 2.4.23-pre7 vmscan.c typo

[PATCH] 2.4.23-pre7 vmscan.c typo

[Shantanu Goel]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]

The following appears to be a typo in mm/vmscan.c

Thanks,
Shantanu

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

[-- Attachment #2: vmscan.c.patch --]
[-- Type: application/octet-stream, Size: 244 bytes --]

--- vmscan.c.~1~	2003-10-19 21:36:26.000000000 -0400
+++ vmscan.c	2003-10-19 21:37:17.000000000 -0400
@@ -596,7 +596,7 @@
 			continue;
 		}
 
-		nr_pages--;
+		ratio--;
 
 		del_page_from_active_list(page);
 		add_page_to_inactive_list(page);

Re: [PATCH] 2.4.23-pre7 vmscan.c typo

[Andrew Morton]

Shantanu Goel <sgoel01@yahoo.com> wrote:
>
> The following appears to be a typo in mm/vmscan.c
> 

It sure is.  Scary.

--- a/mm/vmscan.c	2003-10-19 21:36:26.000000000 -0400
+++ a/mm/vmscan.c	2003-10-19 21:37:17.000000000 -0400
@@ -596,7 +596,7 @@
 			continue;
 		}
 
-		nr_pages--;
+		ratio--;
 
 		del_page_from_active_list(page);
 		add_page_to_inactive_list(page);



I note that `ratio' here is the number of pages which we try to deactivate
rather than the number of pages which we scan.  Is this intentional?

Re: [PATCH] 2.4.23-pre7 vmscan.c typo

[Andrea Arcangeli]

On Sun, Oct 19, 2003 at 10:00:05PM -0700, Andrew Morton wrote:
> Shantanu Goel <sgoel01@yahoo.com> wrote:
> >
> > The following appears to be a typo in mm/vmscan.c
> > 
> 
> It sure is.  Scary.

indeed, great spotting.

> 
> --- a/mm/vmscan.c	2003-10-19 21:36:26.000000000 -0400
> +++ a/mm/vmscan.c	2003-10-19 21:37:17.000000000 -0400
> @@ -596,7 +596,7 @@
>  			continue;
>  		}
>  
> -		nr_pages--;
> +		ratio--;
>  
>  		del_page_from_active_list(page);
>  		add_page_to_inactive_list(page);
> 
> 
> 
> I note that `ratio' here is the number of pages which we try to deactivate
> rather than the number of pages which we scan.  Is this intentional?

yes, it's intentional, this ensures we refile a number of pages and that
we don't only roll the list, it won't loop forever since at the second
pass the referenced bit will be clear.

BTW, the above obviously correct patch was apparently due an half merge
error too, this is what my 2.4.22aa1 or alternatively 2.4.23pre6aa3
looks like in this area. This gets right the highmem case too, so that
we ensure to refile normal zone if the user is GFP_KERNEL and we don't
deactivate highmem unless it's worthwhile, and it has the bh-related
knwoledge, so over time it'd be better to merge these bits too.

thanks,

static void refill_inactive(int nr_pages, zone_t * classzone)
{
	struct list_head * entry;
	unsigned long ratio;

	ratio = (unsigned long) nr_pages * classzone->nr_active_pages / (((unsigned long) classzone->nr_inactive_pages * vm_lru_balance_ratio) + 1);

	entry = active_list.prev;
	while (ratio && entry != &active_list) {
		struct page * page;
		int related_metadata = 0;

		page = list_entry(entry, struct page, lru);
		entry = entry->prev;

		if (!memclass(page_zone(page), classzone)) {
			/*
			 * Hack to address an issue found by Rik. The problem is that
			 * highmem pages can hold buffer headers allocated
			 * from the slab on lowmem, and so if we are working
			 * on the NORMAL classzone here, it is correct not to
			 * try to free the highmem pages themself (that would be useless)
			 * but we must make sure to drop any lowmem metadata related to those
			 * highmem pages.
			 */
			if (page->buffers && page->mapping) { /* fast path racy check */
				if (unlikely(TryLockPage(page)))
					continue;
				if (page->buffers && page->mapping && memclass_related_bhs(page, classzone)) /* non racy check */
					related_metadata = 1;
				UnlockPage(page);
			}
			if (!related_metadata)
				continue;
		}

		if (PageTestandClearReferenced(page)) {
			list_del(&page->lru);
			list_add(&page->lru, &active_list);
			continue;
		}

		if (!related_metadata)
			ratio--;

		del_page_from_active_list(page);
		add_page_to_inactive_list(page);
		SetPageReferenced(page);
	}
	if (entry != &active_list) {
		list_del(&active_list);
		list_add(&active_list, entry);
	}
}

Andrea - If you prefer relying on open source software, check these links:
	    rsync.kernel.org::pub/scm/linux/kernel/bkcvs/linux-2.[45]/
	    http://www.cobite.com/cvsps/

Re: [PATCH] 2.4.23-pre7 vmscan.c typo

[Marcelo Tosatti]

On Mon, 20 Oct 2003, Andrea Arcangeli wrote:

> On Sun, Oct 19, 2003 at 10:00:05PM -0700, Andrew Morton wrote:
> > Shantanu Goel <sgoel01@yahoo.com> wrote:
> > >
> > > The following appears to be a typo in mm/vmscan.c
> > > 
> > 
> > It sure is.  Scary.
> 
> indeed, great spotting.
> 
> > 
> > --- a/mm/vmscan.c	2003-10-19 21:36:26.000000000 -0400
> > +++ a/mm/vmscan.c	2003-10-19 21:37:17.000000000 -0400
> > @@ -596,7 +596,7 @@
> >  			continue;
> >  		}
> >  
> > -		nr_pages--;
> > +		ratio--;
> >  
> >  		del_page_from_active_list(page);
> >  		add_page_to_inactive_list(page);
> > 
> > 
> > 
> > I note that `ratio' here is the number of pages which we try to deactivate
> > rather than the number of pages which we scan.  Is this intentional?
> 
> yes, it's intentional, this ensures we refile a number of pages and that
> we don't only roll the list, it won't loop forever since at the second
> pass the referenced bit will be clear.
> 
> BTW, the above obviously correct patch was apparently due an half merge
> error too, this is what my 2.4.22aa1 or alternatively 2.4.23pre6aa3
> looks like in this area. 

Fixed. Thanks Shantanu. 

It must have been a merge error. :(

> This gets right the highmem case too, so that we ensure to refile normal
> zone if the user is GFP_KERNEL and we don't deactivate highmem unless
> it's worthwhile, and it has the bh-related knwoledge, so over time it'd
> be better to merge these bits too.

Will take a look into this...