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

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

Jay Lan wrote:
> 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.

Just looked at the __vm_stat_account() code. It is enclosed inside
#ifdef CONFIG_PROC_FS.

If that is necessary, i can not put hiwater_vm update code in there. The
system accounting code should not be dependent on a config flag that has
nothing to do with system accounting.

Thanks,
  - jay

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