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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/