Re: [PATCH 1/1] mm: add dirty_highmem option

["Bron Gondwana" <brong@fastmail.fm>]

On Mon, 26 Nov 2007 20:54:28 -0800, "Andrew Morton" <akpm@linux-foundation.org> said:
> On Thu, 22 Nov 2007 14:42:04 +1100 Bron Gondwana <brong@fastmail.fm>
> wrote:
> 
> >  /*
> > + * free highmem will not be subtracted from the total free memory
> > + * for calculating free ratios if vm_dirty_highmem is true
> > + */
> > +int vm_dirty_highmem;
> 
> One would expect that setting dirty_highmem to true would cause highmem
> to
> be accounted in dirty-memory calculations.  However with this change
> reality is in fact the inverse of that.
> 
> So how about this?

Actually, I'm confused now.  Maybe I chose a bad name to begin with.
Does it mean "I am allowed to dirty high memory" or "my high memory
will be dirty if this is on"?

Hmm... I'm even having trouble articulating what's odd about it.

I guess my internal model was: "if this flag is set then you are
allowed to make high memory dirty without needing to flush it
immediately", which is why I made it that way around.


No - you're wrong.  My patch _did_ include high memory in the dirty
memory calculations when dirty_highmem was true.

>  	x = global_page_state(NR_FREE_PAGES)
>  		+ global_page_state(NR_INACTIVE)
>  		+ global_page_state(NR_ACTIVE);

This is the total memory, _including_ high memory.

>           x -= highmem_dirtyable_memory(x);

This removes the high memory from the total count.


I think I got it right.  If dirty_highmem is set to true, then
don't subtract highmem from the total memory count before
calculating the percentages.  That's what I meant, and that's
what the toggle did.  Removed the subtraction.

Bron.


>  Documentation/filesystems/proc.txt |    4 ++--
>  mm/page-writeback.c                |    8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff -puN
> Documentation/filesystems/proc.txt~mm-add-dirty_highmem-option-fix
> Documentation/filesystems/proc.txt
> --- a/Documentation/filesystems/proc.txt~mm-add-dirty_highmem-option-fix
> +++ a/Documentation/filesystems/proc.txt
> @@ -1265,8 +1265,8 @@ Contains, as a boolean, a switch to allo
>  part of the "available" memory against which the dirty ratios will be
>  applied.
>  
> -Setting this to 1 can be useful on 32 bit machines where you want to
> make
> -random changes within an MMAPed file that is larger than your available
> +Setting this to 0 (false) can be useful on 32 bit machines where you
> wish to
> +make random changes within an MMAPed file that is larger than your
> available
>  lowmem, however it is potentially dangerous and has serious
>  bounce-buffer
>  issues.
>  
> diff -puN mm/page-writeback.c~mm-add-dirty_highmem-option-fix
> mm/page-writeback.c
> --- a/mm/page-writeback.c~mm-add-dirty_highmem-option-fix
> +++ a/mm/page-writeback.c
> @@ -69,10 +69,10 @@ static inline long sync_writeback_pages(
>  int dirty_background_ratio = 5;
>  
>  /*
> - * free highmem will not be subtracted from the total free memory
> - * for calculating free ratios if vm_dirty_highmem is true
> + * free highmem will be subtracted from the total free memory for
> calculating
> + * free ratios if vm_dirty_highmem is true
>   */
> -int vm_dirty_highmem;
> +int vm_dirty_highmem = 1;
>  
>  /*
>   * The generator of dirty data starts writeback at this percentage
> @@ -293,7 +293,7 @@ static unsigned long determine_dirtyable
>  	x = global_page_state(NR_FREE_PAGES)
>  		+ global_page_state(NR_INACTIVE)
>  		+ global_page_state(NR_ACTIVE);
> -	if (!vm_dirty_highmem)
> +	if (vm_dirty_highmem)
>  		x -= highmem_dirtyable_memory(x);
>  	return x + 1;	/* Ensure that we never return 0 */
>  }
> _
> 
> 
> 
> 
> (I dropped the already-merged part of your patch)
> 
> (I fixed a build error in kernel/sysctl.c: "one" was defined twice when
> suitable config options were set).
> 
> (It's an unpleasing patch, btw.  But it's an unpleasant problem and at
> least
> this way people can tell us "hey, I did <this> and it started to work")
-- 
  Bron Gondwana
  brong@fastmail.fm