[PATCH] mm: Delete non-atomic mm counter implementation

[PATCH] mm: Delete non-atomic mm counter implementation

[Matt Fleming]

From: Matt Fleming <matt.fleming@linux.intel.com>

The problem with having two different types of counters is that
developers adding new code need to keep in mind whether it's safe to
use both the atomic and non-atomic implementations. For example, when
adding new callers of the *_mm_counter() functions a developer needs
to ensure that those paths are always executed with page_table_lock
held, in case we're using the non-atomic implementation of mm
counters.

Hugh Dickins introduced the atomic mm counters in commit f412ac08c986
("[PATCH] mm: fix rss and mmlist locking"). When asked why he left the
non-atomic counters around he said,

  | The only reason was to avoid adding costly atomic operations into a
  | configuration that had no need for them there: the page_table_lock
  | sufficed.
  |
  | Certainly it would be simpler just to delete the non-atomic variant.
  |
  | And I think it's fair to say that any configuration on which we're
  | measuring performance to that degree (rather than "does it boot fast?"
  | type measurements), would already be going the split ptlocks route.

Removing the non-atomic counters eases the maintenance burden because
developers no longer have to mindful of the two implementations when
using *_mm_counter().

Note that all architectures provide a means of atomically updating
atomic_long_t variables, even if they have to revert to the generic
spinlock implementation because they don't support 64-bit atomic
instructions (see lib/atomic64.c).

Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---

Dave, you might want to take this into your pagetable counters series
so that you only need to worry about atomic mm counters.

 include/linux/mm.h       |   44 +++++++-------------------------------------
 include/linux/mm_types.h |    9 +++------
 2 files changed, 10 insertions(+), 43 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index dd87a78..ee64af2 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1034,65 +1034,35 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 /*
  * per-process(per-mm_struct) statistics.
  */
-#if defined(SPLIT_RSS_COUNTING)
-/*
- * The mm counters are not protected by its page_table_lock,
- * so must be incremented atomically.
- */
 static inline void set_mm_counter(struct mm_struct *mm, int member, long value)
 {
 	atomic_long_set(&mm->rss_stat.count[member], value);
 }
 
+#if defined(SPLIT_RSS_COUNTING)
 unsigned long get_mm_counter(struct mm_struct *mm, int member);
-
-static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
-{
-	atomic_long_add(value, &mm->rss_stat.count[member]);
-}
-
-static inline void inc_mm_counter(struct mm_struct *mm, int member)
-{
-	atomic_long_inc(&mm->rss_stat.count[member]);
-}
-
-static inline void dec_mm_counter(struct mm_struct *mm, int member)
-{
-	atomic_long_dec(&mm->rss_stat.count[member]);
-}
-
-#else  /* !USE_SPLIT_PTLOCKS */
-/*
- * The mm counters are protected by its page_table_lock,
- * so can be incremented directly.
- */
-static inline void set_mm_counter(struct mm_struct *mm, int member, long value)
-{
-	mm->rss_stat.count[member] = value;
-}
-
+#else
 static inline unsigned long get_mm_counter(struct mm_struct *mm, int member)
 {
-	return mm->rss_stat.count[member];
+	return atomic_long_read(&mm->rss_stat.count[member]);
 }
+#endif
 
 static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
 {
-	mm->rss_stat.count[member] += value;
+	atomic_long_add(value, &mm->rss_stat.count[member]);
 }
 
 static inline void inc_mm_counter(struct mm_struct *mm, int member)
 {
-	mm->rss_stat.count[member]++;
+	atomic_long_inc(&mm->rss_stat.count[member]);
 }
 
 static inline void dec_mm_counter(struct mm_struct *mm, int member)
 {
-	mm->rss_stat.count[member]--;
+	atomic_long_dec(&mm->rss_stat.count[member]);
 }
 
-#endif /* !USE_SPLIT_PTLOCKS */
-
 static inline unsigned long get_mm_rss(struct mm_struct *mm)
 {
 	return get_mm_counter(mm, MM_FILEPAGES) +
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index ca01ab2..b8ca318 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -205,19 +205,16 @@ enum {
 
 #if USE_SPLIT_PTLOCKS && defined(CONFIG_MMU)
 #define SPLIT_RSS_COUNTING
-struct mm_rss_stat {
-	atomic_long_t count[NR_MM_COUNTERS];
-};
 /* per-thread cached information, */
 struct task_rss_stat {
 	int events;	/* for synchronization threshold */
 	int count[NR_MM_COUNTERS];
 };
-#else  /* !USE_SPLIT_PTLOCKS */
+#endif /* USE_SPLIT_PTLOCKS */
+
 struct mm_rss_stat {
-	unsigned long count[NR_MM_COUNTERS];
+	atomic_long_t count[NR_MM_COUNTERS];
 };
-#endif /* !USE_SPLIT_PTLOCKS */
 
 struct mm_struct {
 	struct vm_area_struct * mmap;		/* list of VMAs */
-- 
1.7.4.4

Re: [PATCH] mm: Delete non-atomic mm counter implementation

[Matt Fleming]

[Oops! Replying with the correct linux-mm address]

On Wed, 27 Apr 2011 15:36:05 +0100
Matt Fleming <matt@console-pimps.org> wrote:

> From: Matt Fleming <matt.fleming@linux.intel.com>
> 
> The problem with having two different types of counters is that
> developers adding new code need to keep in mind whether it's safe to
> use both the atomic and non-atomic implementations. For example, when
> adding new callers of the *_mm_counter() functions a developer needs
> to ensure that those paths are always executed with page_table_lock
> held, in case we're using the non-atomic implementation of mm
> counters.
> 
> Hugh Dickins introduced the atomic mm counters in commit f412ac08c986
> ("[PATCH] mm: fix rss and mmlist locking"). When asked why he left the
> non-atomic counters around he said,
> 
>   | The only reason was to avoid adding costly atomic operations into a
>   | configuration that had no need for them there: the page_table_lock
>   | sufficed.
>   |
>   | Certainly it would be simpler just to delete the non-atomic variant.
>   |
>   | And I think it's fair to say that any configuration on which we're
>   | measuring performance to that degree (rather than "does it boot fast?"
>   | type measurements), would already be going the split ptlocks route.
> 
> Removing the non-atomic counters eases the maintenance burden because
> developers no longer have to mindful of the two implementations when
> using *_mm_counter().
> 
> Note that all architectures provide a means of atomically updating
> atomic_long_t variables, even if they have to revert to the generic
> spinlock implementation because they don't support 64-bit atomic
> instructions (see lib/atomic64.c).
> 
> Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
> ---
> 
> Dave, you might want to take this into your pagetable counters series
> so that you only need to worry about atomic mm counters.
> 
>  include/linux/mm.h       |   44 +++++++-------------------------------------
>  include/linux/mm_types.h |    9 +++------
>  2 files changed, 10 insertions(+), 43 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index dd87a78..ee64af2 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1034,65 +1034,35 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>  /*
>   * per-process(per-mm_struct) statistics.
>   */
> -#if defined(SPLIT_RSS_COUNTING)
> -/*
> - * The mm counters are not protected by its page_table_lock,
> - * so must be incremented atomically.
> - */
>  static inline void set_mm_counter(struct mm_struct *mm, int member, long value)
>  {
>  	atomic_long_set(&mm->rss_stat.count[member], value);
>  }
>  
> +#if defined(SPLIT_RSS_COUNTING)
>  unsigned long get_mm_counter(struct mm_struct *mm, int member);
> -
> -static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
> -{
> -	atomic_long_add(value, &mm->rss_stat.count[member]);
> -}
> -
> -static inline void inc_mm_counter(struct mm_struct *mm, int member)
> -{
> -	atomic_long_inc(&mm->rss_stat.count[member]);
> -}
> -
> -static inline void dec_mm_counter(struct mm_struct *mm, int member)
> -{
> -	atomic_long_dec(&mm->rss_stat.count[member]);
> -}
> -
> -#else  /* !USE_SPLIT_PTLOCKS */
> -/*
> - * The mm counters are protected by its page_table_lock,
> - * so can be incremented directly.
> - */
> -static inline void set_mm_counter(struct mm_struct *mm, int member, long value)
> -{
> -	mm->rss_stat.count[member] = value;
> -}
> -
> +#else
>  static inline unsigned long get_mm_counter(struct mm_struct *mm, int member)
>  {
> -	return mm->rss_stat.count[member];
> +	return atomic_long_read(&mm->rss_stat.count[member]);
>  }
> +#endif
>  
>  static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
>  {
> -	mm->rss_stat.count[member] += value;
> +	atomic_long_add(value, &mm->rss_stat.count[member]);
>  }
>  
>  static inline void inc_mm_counter(struct mm_struct *mm, int member)
>  {
> -	mm->rss_stat.count[member]++;
> +	atomic_long_inc(&mm->rss_stat.count[member]);
>  }
>  
>  static inline void dec_mm_counter(struct mm_struct *mm, int member)
>  {
> -	mm->rss_stat.count[member]--;
> +	atomic_long_dec(&mm->rss_stat.count[member]);
>  }
>  
> -#endif /* !USE_SPLIT_PTLOCKS */
> -
>  static inline unsigned long get_mm_rss(struct mm_struct *mm)
>  {
>  	return get_mm_counter(mm, MM_FILEPAGES) +
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index ca01ab2..b8ca318 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -205,19 +205,16 @@ enum {
>  
>  #if USE_SPLIT_PTLOCKS && defined(CONFIG_MMU)
>  #define SPLIT_RSS_COUNTING
> -struct mm_rss_stat {
> -	atomic_long_t count[NR_MM_COUNTERS];
> -};
>  /* per-thread cached information, */
>  struct task_rss_stat {
>  	int events;	/* for synchronization threshold */
>  	int count[NR_MM_COUNTERS];
>  };
> -#else  /* !USE_SPLIT_PTLOCKS */
> +#endif /* USE_SPLIT_PTLOCKS */
> +
>  struct mm_rss_stat {
> -	unsigned long count[NR_MM_COUNTERS];
> +	atomic_long_t count[NR_MM_COUNTERS];
>  };
> -#endif /* !USE_SPLIT_PTLOCKS */
>  
>  struct mm_struct {
>  	struct vm_area_struct * mmap;		/* list of VMAs */



-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] mm: Delete non-atomic mm counter implementation

[Dave Hansen]

On Wed, 2011-04-27 at 15:36 +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@linux.intel.com>
> 
> The problem with having two different types of counters is that
> developers adding new code need to keep in mind whether it's safe to
> use both the atomic and non-atomic implementations. For example, when
> adding new callers of the *_mm_counter() functions a developer needs
> to ensure that those paths are always executed with page_table_lock
> held, in case we're using the non-atomic implementation of mm
> counters.

> Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
> ---
> 
> Dave, you might want to take this into your pagetable counters series
> so that you only need to worry about atomic mm counters.

Thanks for doing the patch.  I'll certainly pick it up and submit it
alongside my stuff unless Andrew picks it up first.

Acked-by: Dave Hansen <dave@linux.vnet.ibm.com>


-- Dave

Re: [PATCH] mm: Delete non-atomic mm counter implementation

[KAMEZAWA Hiroyuki]

On Wed, 27 Apr 2011 15:36:05 +0100
Matt Fleming <matt@console-pimps.org> wrote:

> From: Matt Fleming <matt.fleming@linux.intel.com>
> 
> The problem with having two different types of counters is that
> developers adding new code need to keep in mind whether it's safe to
> use both the atomic and non-atomic implementations. For example, when
> adding new callers of the *_mm_counter() functions a developer needs
> to ensure that those paths are always executed with page_table_lock
> held, in case we're using the non-atomic implementation of mm
> counters.
> 
> Hugh Dickins introduced the atomic mm counters in commit f412ac08c986
> ("[PATCH] mm: fix rss and mmlist locking"). When asked why he left the
> non-atomic counters around he said,
> 
>   | The only reason was to avoid adding costly atomic operations into a
>   | configuration that had no need for them there: the page_table_lock
>   | sufficed.
>   |
>   | Certainly it would be simpler just to delete the non-atomic variant.
>   |
>   | And I think it's fair to say that any configuration on which we're
>   | measuring performance to that degree (rather than "does it boot fast?"
>   | type measurements), would already be going the split ptlocks route.
> 
> Removing the non-atomic counters eases the maintenance burden because
> developers no longer have to mindful of the two implementations when
> using *_mm_counter().
> 
> Note that all architectures provide a means of atomically updating
> atomic_long_t variables, even if they have to revert to the generic
> spinlock implementation because they don't support 64-bit atomic
> instructions (see lib/atomic64.c).
> 
> Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>

now mm_counters are update in per-thread and removing non-atomic
will not have big impact in performance, I hope. 

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH] mm: Delete non-atomic mm counter implementation

[Matt Fleming]

On Thu, 28 Apr 2011 09:08:57 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
 
> now mm_counters are update in per-thread and removing non-atomic
> will not have big impact in performance, I hope. 
> 
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Thanks!

Note that this patch doesn't enable per-thread counters for the
non-USE_SPLIT_PTLOCKS case; SPLIT_RSS_COUNTING is still only enabled if
USE_SPLIT_PTLOCKS is on.

-- 
Matt Fleming, Intel Open Source Technology Center