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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]
  Powered by Linux