linux-kernel - Re: [PATCH 2/3] Memory management livelock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20081001225404.4e973465.akpm@linux-foundation.org>
Date:	Wed, 1 Oct 2008 22:54:04 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Mikulas Patocka <mpatocka@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-mm@...r.kernel.org,
	agk@...hat.com, mbroz@...hat.com, chris@...chsys.com
Subject: Re: [PATCH 2/3] Memory management livelock


> Subject: [PATCH 2/3] Memory management livelock

Please don't send multiple patches with identical titles - think up a
good, unique, meaningful title for each patch.

On Wed, 24 Sep 2008 14:52:18 -0400 (EDT) Mikulas Patocka <mpatocka@...hat.com> wrote:

> Avoid starvation when walking address space.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@...hat.com>
> 

Please include a full changelog with each iteration of each patch.

That changelog should explain the reason for playing games with
bitlocks so Linus doesn't have kittens when he sees it.

>  include/linux/pagemap.h |    1 +
>  mm/filemap.c            |   20 ++++++++++++++++++++
>  mm/page-writeback.c     |   37 ++++++++++++++++++++++++++++++++++++-
>  mm/truncate.c           |   24 +++++++++++++++++++++++-
>  4 files changed, 80 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6.27-rc7-devel/include/linux/pagemap.h
> ===================================================================
> --- linux-2.6.27-rc7-devel.orig/include/linux/pagemap.h	2008-09-24 02:57:37.000000000 +0200
> +++ linux-2.6.27-rc7-devel/include/linux/pagemap.h	2008-09-24 02:59:04.000000000 +0200
> @@ -21,6 +21,7 @@
>  #define	AS_EIO		(__GFP_BITS_SHIFT + 0)	/* IO error on async write */
>  #define AS_ENOSPC	(__GFP_BITS_SHIFT + 1)	/* ENOSPC on async write */
>  #define AS_MM_ALL_LOCKS	(__GFP_BITS_SHIFT + 2)	/* under mm_take_all_locks() */
> +#define AS_STARVATION	(__GFP_BITS_SHIFT + 3)	/* an anti-starvation barrier */
>  
>  static inline void mapping_set_error(struct address_space *mapping, int error)
>  {
> Index: linux-2.6.27-rc7-devel/mm/filemap.c
> ===================================================================
> --- linux-2.6.27-rc7-devel.orig/mm/filemap.c	2008-09-24 02:59:33.000000000 +0200
> +++ linux-2.6.27-rc7-devel/mm/filemap.c	2008-09-24 03:13:47.000000000 +0200
> @@ -269,10 +269,19 @@ int wait_on_page_writeback_range(struct 
>  	int nr_pages;
>  	int ret = 0;
>  	pgoff_t index;
> +	long pages_to_process;
>  
>  	if (end < start)
>  		return 0;
>  
> +	/*
> +	 * Estimate the number of pages to process. If we process significantly
> +	 * more than this, someone is making writeback pages under us.
> +	 * We must pull the anti-starvation plug.
> +	 */
> +	pages_to_process = bdi_stat(mapping->backing_dev_info, BDI_WRITEBACK);
> +	pages_to_process += (pages_to_process >> 3) + 16;

This sequence appears twice and it would probably be clearer to
implement it in a well-commented function.

>  	pagevec_init(&pvec, 0);
>  	index = start;
>  	while ((index <= end) &&
> @@ -288,6 +297,10 @@ int wait_on_page_writeback_range(struct 
>  			if (page->index > end)
>  				continue;
>  
> +			if (pages_to_process >= 0)
> +				if (!pages_to_process--)
> +					wait_on_bit_lock(&mapping->flags, AS_STARVATION, wait_action_schedule, TASK_UNINTERRUPTIBLE);

This is copied three times and perhaps also should be factored out.

Please note that an effort has been made to make mm/filemap.c look
presentable in an 80-col display.

>  			wait_on_page_writeback(page);
>  			if (PageError(page))
>  				ret = -EIO;
> @@ -296,6 +309,13 @@ int wait_on_page_writeback_range(struct 
>  		cond_resched();
>  	}
>  
> +	if (pages_to_process < 0) {
> +		smp_mb__before_clear_bit();
> +		clear_bit(AS_STARVATION, &mapping->flags);
> +		smp_mb__after_clear_bit();
> +		wake_up_bit(&mapping->flags, AS_STARVATION);
> +	}

This sequence is repeated three or four times and should be pulled out
into a well-commented function.  That comment should explain the logic
behind the use of these barriers, please.


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