[PATCH] RSS container, fix freeing of active pages

[PATCH] RSS container, fix freeing of active pages

[Balbir Singh]

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

Hi, Pavel,

I missed you on the list and the patch did not make it to lkml.
I am resending the patch.



-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

[-- Attachment #2: rss-fix-free-of-active-pages.patch --]
[-- Type: text/x-patch, Size: 2463 bytes --]



This patch fixes the bad_page() warning seen while freeing a container page.
By default all container pages are added to the active list on the
container. This patch lazily moves pages to the right list, so that a page
on the active list of the zone LRU and the inactive list of the container
does not get freed. If that happens, we see a warning from bad_page().

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 linux/rss_container.h |    0 
 mm/rss_container.c    |   30 ++++++++++++++++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff -puN mm/rss_container.c~rss-fix-free-of-active-pages mm/rss_container.c
--- linux-2.6.20/mm/rss_container.c~rss-fix-free-of-active-pages	2007-04-25 22:05:54.000000000 +0530
+++ linux-2.6.20-balbir/mm/rss_container.c	2007-04-25 23:12:35.000000000 +0530
@@ -119,18 +119,36 @@ void container_rss_move_lists(struct pag
 }
 
 static unsigned long isolate_container_pages(unsigned long nr_to_scan,
-		struct list_head *src, struct list_head *dst,
-		unsigned long *scanned, struct zone *zone)
+		struct rss_container *rss, struct list_head *dst,
+		unsigned long *scanned, struct zone *zone, bool active)
 {
 	unsigned long nr_taken = 0;
 	struct page *page;
 	struct page_container *pc;
 	unsigned long scan;
+	struct list_head *src;
 	LIST_HEAD(pc_list);
 
+	src = active ? &rss->active_list : &rss->inactive_list;
+
 	for (scan = 0; scan < nr_to_scan && !list_empty(src); scan++) {
 		pc = list_entry(src->prev, struct page_container, list);
 		page = pc->page;
+
+		/*
+		 * We might have got our active, inactive lists
+		 * incorrect, fix it here
+		 */
+		if (active && !PageActive(page)) {
+			list_move(&pc->list, &rss->inactive_list);
+			scan--;
+			continue;
+		} else if (!active && PageActive(page)) {
+			list_move(&pc->list, &rss->active_list);
+			scan--;
+			continue;
+		}
+
 		if (page_zone(page) != zone)
 			continue;
 
@@ -158,12 +176,8 @@ unsigned long isolate_pages_in_container
 	unsigned long ret;
 
 	spin_lock(&rss->res.lock);
-	if (active)
-		ret = isolate_container_pages(nr_to_scan, &rss->active_list,
-				dst, scanned, zone);
-	else
-		ret = isolate_container_pages(nr_to_scan, &rss->inactive_list,
-				dst, scanned, zone);
+	ret = isolate_container_pages(nr_to_scan, rss, dst, scanned, zone,
+					active);
 	spin_unlock(&rss->res.lock);
 	return ret;
 }
diff -puN include/linux/rss_container.h~rss-fix-free-of-active-pages include/linux/rss_container.h
_

Re: [PATCH] RSS container, fix freeing of active pages

[Pavel Emelianov]

Balbir Singh wrote:
> Hi, Pavel,
> 
> I missed you on the list and the patch did not make it to lkml.
> I am resending the patch.
> 
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 
> 
> This patch fixes the bad_page() warning seen while freeing a container page.
> By default all container pages are added to the active list on the
> container. This patch lazily moves pages to the right list, so that a page
> on the active list of the zone LRU and the inactive list of the container
> does not get freed. If that happens, we see a warning from bad_page().
> 
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
> 
>  linux/rss_container.h |    0 
>  mm/rss_container.c    |   30 ++++++++++++++++++++++--------
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff -puN mm/rss_container.c~rss-fix-free-of-active-pages mm/rss_container.c
> --- linux-2.6.20/mm/rss_container.c~rss-fix-free-of-active-pages	2007-04-25 22:05:54.000000000 +0530
> +++ linux-2.6.20-balbir/mm/rss_container.c	2007-04-25 23:12:35.000000000 +0530
> @@ -119,18 +119,36 @@ void container_rss_move_lists(struct pag
>  }
>  
>  static unsigned long isolate_container_pages(unsigned long nr_to_scan,
> -		struct list_head *src, struct list_head *dst,
> -		unsigned long *scanned, struct zone *zone)
> +		struct rss_container *rss, struct list_head *dst,
> +		unsigned long *scanned, struct zone *zone, bool active)
>  {
>  	unsigned long nr_taken = 0;
>  	struct page *page;
>  	struct page_container *pc;
>  	unsigned long scan;
> +	struct list_head *src;
>  	LIST_HEAD(pc_list);
>  
> +	src = active ? &rss->active_list : &rss->inactive_list;
> +
>  	for (scan = 0; scan < nr_to_scan && !list_empty(src); scan++) {
>  		pc = list_entry(src->prev, struct page_container, list);
>  		page = pc->page;
> +
> +		/*
> +		 * We might have got our active, inactive lists
> +		 * incorrect, fix it here
> +		 */

Hm... Did you see such a situation in your experiments?
If yes, then why not fix it in a normal way by moving the 
pages from list to list where appropriate?

> +		if (active && !PageActive(page)) {
> +			list_move(&pc->list, &rss->inactive_list);
> +			scan--;
> +			continue;
> +		} else if (!active && PageActive(page)) {
> +			list_move(&pc->list, &rss->active_list);
> +			scan--;
> +			continue;
> +		}
> +
>  		if (page_zone(page) != zone)
>  			continue;
>  
> @@ -158,12 +176,8 @@ unsigned long isolate_pages_in_container
>  	unsigned long ret;
>  
>  	spin_lock(&rss->res.lock);
> -	if (active)
> -		ret = isolate_container_pages(nr_to_scan, &rss->active_list,
> -				dst, scanned, zone);
> -	else
> -		ret = isolate_container_pages(nr_to_scan, &rss->inactive_list,
> -				dst, scanned, zone);
> +	ret = isolate_container_pages(nr_to_scan, rss, dst, scanned, zone,
> +					active);
>  	spin_unlock(&rss->res.lock);
>  	return ret;
>  }
> diff -puN include/linux/rss_container.h~rss-fix-free-of-active-pages include/linux/rss_container.h
> _

Re: [PATCH] RSS container, fix freeing of active pages

[Balbir Singh]

Pavel Emelianov wrote:

> Hm... Did you see such a situation in your experiments?

Yes, we did. We saw a bad_page() error on the console. A page
was freed with the active bit set.

> If yes, then why not fix it in a normal way by moving the 
> pages from list to list where appropriate?
> 

If we add the pages to the appropriate list at the time of page_add_*rmap(),
we would end up with too many code changes split
all over the place. In many cases when we add a page (install_page()
for example), we just release any existing mapping. We would not know
where to add the page -- active list or inactive list?

I am not sure if that was your question in the first place :-)


-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL