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 Tue, 27 Sep 2005, Jay Lan wrote:
>  
>>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?
>>    
>
>Probably not.  Partly because of the PROCFS issue you noticed after.
>And partly because that's a long list, whereas the evidence is that
>__vm_stat_account is nowadays being called in the places that need it.
>
>What, you couldn't find the call to __vm_stat_account next to
>updating total_vm in do_mmap_pgoff :-?  And I don't see that total_vm
>is updated in do_munmap, that and the vm_stat_account have to be done
>at a lower, per-vma level.  Haven't looked at the others yet.
>  

There are two do_mmap_pgoff, one in mm/mmap.c and the other in
mm/nommu.c. It was the mm/nommu.c version that does not  have
a call to __vm_stat_account.

I am not concerned about do_munmap any more from updating
hiwater_vm's concern since the total_vm gets decreased, not increased.

>I think, better leave hiwater_vm to me for now.  Since I've got that
>  
Sounds good to me.

I agreed with you (and Frank) that it makes sense to split hiwater_vm and
hiwater_rss update.
>patchset in the next -mm, it's maybe easier for me to put something
>together on top of that.  And it seems quite possible (haven't checked
>yet) that actually we should be moving the update of total_vm in to the
>core from out all over, rather than splattering hiwater_vm updates about
>  
>I'll take a look in the next couple of days.
>
>Hugh
>  

In case you are going to turn hiwater_vm update into a macro and hide it
in a config flag, here is what i have that might be useful.

Thanks,
 - jay


Index: linux/init/Kconfig
===================================================================
--- linux.orig/init/Kconfig     2005-09-22 14:14:54.105262867 -0700
+++ linux/init/Kconfig  2005-09-22 14:57:19.326784560 -0700
@@ -162,6 +162,19 @@ config BSD_PROCESS_ACCT_V3
          for processing it. A preliminary version of these tools is
available
          at <http://www.physik3.uni-rostock.de/tim/kernel/utils/acct/>.
 
+config ENHANCED_SYS_ACCT
+       bool "Enhanced System Accounting support"
+       depends on BSD_PROCESS_ACCT
+       default n
+       help
+         If you say Y here, some extra system accounting data will be
+         collected and updated to task struct as well as task->mm struct.
+         These information are used and presented in enhanced system
+         accounting packages such as Comprehensive System Accounting (CSA).
+         This activity may force cachelines into shared from exclusive
+         on a processor. The CSA project can be viewed at
+         <http://oss.sgi.com/projects/csa/>.
+
 config SYSCTL
        bool "Sysctl support"
        ---help---
Index: linux/mm/memory.c
===================================================================
--- linux.orig/mm/memory.c      2005-09-22 14:15:07.465757425 -0700
+++ linux/mm/memory.c   2005-09-27 16:55:28.438702812 -0700
@@ -2206,21 +2206,21 @@ unsigned long vmalloc_to_pfn(void * vmal
 
 EXPORT_SYMBOL(vmalloc_to_pfn);
 
+#ifdef CONFIG_ENHANCED_SYS_ACCT
 /*
- * update_mem_hiwater
- *     - update per process rss and vm high water data
+ * update_rss_hiwater
+ *     - update per process rss high water data
  */
-void update_mem_hiwater(struct task_struct *tsk)
+void update_rss_hiwater(struct mm_struct *mm)
 {
-       if (tsk->mm) {
-               unsigned long rss = get_mm_counter(tsk->mm, rss);
+       if (likely(mm)) {
+               unsigned long rss = get_mm_counter(mm, rss);
 
-               if (tsk->mm->hiwater_rss < rss)
-                       tsk->mm->hiwater_rss = rss;
-               if (tsk->mm->hiwater_vm < tsk->mm->total_vm)
-                       tsk->mm->hiwater_vm = tsk->mm->total_vm;
+               if (mm->hiwater_rss < rss)
+                       mm->hiwater_rss = rss;
        }
 }
+#endif
 
 #if !defined(__HAVE_ARCH_GATE_AREA)
 
Index: linux/include/linux/mm.h
===================================================================
--- linux.orig/include/linux/mm.h       2005-08-28 16:41:01.000000000 -0700
+++ linux/include/linux/mm.h    2005-09-27 17:33:49.372846334 -0700
@@ -949,7 +949,18 @@ static inline void vm_stat_unaccount(str
 }
 
 /* update per process rss and vm hiwater data */
-extern void update_mem_hiwater(struct task_struct *tsk);
+#ifdef CONFIG_ENHANCED_SYS_ACCT
+static inline void update_hiwater_vm(struct mm_struct *mm)
+{
+       if (mm->hiwater_vm < mm->total_vm)
+               mm->hiwater_vm = mm->total_vm;
+}
+
+extern void update_hiwater_rss(struct mm_struct *mm);
+#else
+#define update_hiwater_vm(x)           do { } while (0)
+#define update_hiwater_rss(x)          do { } while (0)
+#endif
 
 #ifndef CONFIG_DEBUG_PAGEALLOC
 static inline void

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