Re: [PATCH 2.6.14-rc2] fix incorrect mm->hiwater_vm and mm->hiwater_rss

[Jay Lan <jlan@engr.sgi.com>]

Hugh Dickins wrote:
> On Wed, 21 Sep 2005, Frank van Maarseveen wrote:
> 
>>What about calling
>>
>>static inline void grow_total_vm(struct mm_struct *mm, unsigned long increase)
>>{
>>	mm->total_vm += increase;
>>	if (mm->total_vm > mm->hiwater_vm)
>>		mm->hiwater_vm = mm->total_vm;
>>}
>>
>>whenever total_vm is increased and possibly doing something similar for rss at
>>different places? If it is not on the fast path then it's not necessary to
>>#ifdef the thing anywhere.
> 
> 
> I think there's a good argument for separating hiwater_vm and hiwater_rss
> completely (and you don't seem to be interested in hiwater_rss yourself).
> 
> hiwater_rss is on some fast paths: well, I don't see them as fast paths
> myself (the page faults), but they are of exceptional concern to Christoph,
> and the less we have to mess with struct mm at those points the happier he
> is.  I guess hiwater_rss should remain updated from the timer tick for now.
> 
> But I think you're right that hiwater_vm is best updated where total_vm
> is: I'm not sure if it covers all cases completely (I think there's one
> or two places which don't bother to call __vm_stat_account because they
> believe it won't change anything), but in principle it would make lots of
> sense to do it in the __vm_stat_account which typically follows adjusting
> total_vm, as you did, and if possible nowhere else; rather than adding
> your inline above.

While in the work on separating hiwater_vm from hiwater_rss, i noticed
that __vm_stat_account() was not called in these functions where
total_vm was updated:
     mm/mmap.c                           do_brk
     mm/nommu.c                          do_mmap_pgoff
     mm/nommu.c                          do_munmap
     arch/ppc64/kernel/vdso.c            arch_setup_additional_pages
     arch/x86_64/ia32/syscall32.c        syscall32_setup_pages

Frank tried to touch the latter two in his proposed patch.
Does it make sense we add __vm_stat_account() calls to the above
routines?

- jay


> 
> Would you be satisfied with that, Christoph?
> 
> I should warn you that I'll shortly (shortly meaning in days rather
> than hours) be sending Andrew a patch which will remove the "__" from
> __vm_stat_account, since the old vm_stat_account is now hardly used.
> I'm also rearranging the rss,anon_rss accounting.  Maybe come back
> to the hiwaters later on?
> 
> Hugh