[PATCH 5/8] mm/slub: Factor out some common code.

[PATCH 5/8] mm/slub: Factor out some common code.

[George Spelvin]

For sysfs files that map a boolean to a flags bit.
---
 mm/slub.c |   93 ++++++++++++++++++++++++++++--------------------------------
 1 files changed, 43 insertions(+), 50 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index e15aa7f..856246f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3982,38 +3982,61 @@ static ssize_t objects_partial_show(struct kmem_cache *s, char *buf)
 }
 SLAB_ATTR_RO(objects_partial);
 
+static ssize_t flag_show(struct kmem_cache *s, char *buf, unsigned flag)
+{
+	return sprintf(buf, "%d\n", !!(s->flags & flag));
+}
+
+static ssize_t flag_store(struct kmem_cache *s,
+				const char *buf, size_t length, unsigned flag)
+{
+	s->flags &= ~flag;
+	if (buf[0] == '1')
+		s->flags |= flag;
+	return length;
+}
+
+/* Like above, but changes allocation size; so only allowed on empty slab */
+static ssize_t flag_store_sizechange(struct kmem_cache *s,
+				const char *buf, size_t length, unsigned flag)
+{
+	if (any_slab_objects(s))
+		return -EBUSY;
+
+	flag_store(s, buf, length, flag);
+	calculate_sizes(s, -1);
+	return length;
+}
+
 static ssize_t reclaim_account_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_RECLAIM_ACCOUNT));
+	return flag_show(s, buf, SLAB_RECLAIM_ACCOUNT);
 }
 
 static ssize_t reclaim_account_store(struct kmem_cache *s,
 				const char *buf, size_t length)
 {
-	s->flags &= ~SLAB_RECLAIM_ACCOUNT;
-	if (buf[0] == '1')
-		s->flags |= SLAB_RECLAIM_ACCOUNT;
-	return length;
+	return flag_store(s, buf, length, SLAB_RECLAIM_ACCOUNT);
 }
 SLAB_ATTR(reclaim_account);
 
 static ssize_t hwcache_align_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_HWCACHE_ALIGN));
+	return flag_show(s, buf, SLAB_HWCACHE_ALIGN);
 }
 SLAB_ATTR_RO(hwcache_align);
 
 #ifdef CONFIG_ZONE_DMA
 static ssize_t cache_dma_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_CACHE_DMA));
+	return flag_show(s, buf, SLAB_CACHE_DMA);
 }
 SLAB_ATTR_RO(cache_dma);
 #endif
 
 static ssize_t destroy_by_rcu_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_DESTROY_BY_RCU));
+	return flag_show(s, buf, SLAB_DESTROY_BY_RCU);
 }
 SLAB_ATTR_RO(destroy_by_rcu);
 
@@ -4032,88 +4055,61 @@ SLAB_ATTR_RO(total_objects);
 
 static ssize_t sanity_checks_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_DEBUG_FREE));
+	return flag_show(s, buf, SLAB_DEBUG_FREE);
 }
 
 static ssize_t sanity_checks_store(struct kmem_cache *s,
 				const char *buf, size_t length)
 {
-	s->flags &= ~SLAB_DEBUG_FREE;
-	if (buf[0] == '1')
-		s->flags |= SLAB_DEBUG_FREE;
-	return length;
+	return flag_store(s, buf, length, SLAB_DEBUG_FREE);
 }
 SLAB_ATTR(sanity_checks);
 
 static ssize_t trace_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_TRACE));
+	return flag_show(s, buf, SLAB_TRACE);
 }
 
 static ssize_t trace_store(struct kmem_cache *s, const char *buf,
 							size_t length)
 {
-	s->flags &= ~SLAB_TRACE;
-	if (buf[0] == '1')
-		s->flags |= SLAB_TRACE;
-	return length;
+	return flag_store(s, buf, length, SLAB_TRACE);
 }
 SLAB_ATTR(trace);
 
 static ssize_t red_zone_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_RED_ZONE));
+	return flag_show(s, buf, SLAB_RED_ZONE);
 }
 
 static ssize_t red_zone_store(struct kmem_cache *s,
 				const char *buf, size_t length)
 {
-	if (any_slab_objects(s))
-		return -EBUSY;
-
-	s->flags &= ~SLAB_RED_ZONE;
-	if (buf[0] == '1')
-		s->flags |= SLAB_RED_ZONE;
-	calculate_sizes(s, -1);
-	return length;
+	return flag_store_sizechange(s, buf, length, SLAB_RED_ZONE);
 }
 SLAB_ATTR(red_zone);
 
 static ssize_t poison_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_POISON));
+	return flag_show(s, buf, SLAB_POISON);
 }
 
 static ssize_t poison_store(struct kmem_cache *s,
 				const char *buf, size_t length)
 {
-	if (any_slab_objects(s))
-		return -EBUSY;
-
-	s->flags &= ~SLAB_POISON;
-	if (buf[0] == '1')
-		s->flags |= SLAB_POISON;
-	calculate_sizes(s, -1);
-	return length;
+	return flag_store_sizechange(s, buf, length, SLAB_POISON);
 }
 SLAB_ATTR(poison);
 
 static ssize_t store_user_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_STORE_USER));
+	return flag_show(s, buf, SLAB_STORE_USER);
 }
 
 static ssize_t store_user_store(struct kmem_cache *s,
 				const char *buf, size_t length)
 {
-	if (any_slab_objects(s))
-		return -EBUSY;
-
-	s->flags &= ~SLAB_STORE_USER;
-	if (buf[0] == '1')
-		s->flags |= SLAB_STORE_USER;
-	calculate_sizes(s, -1);
-	return length;
+	return flag_store_sizechange(s, buf, length, SLAB_STORE_USER);
 }
 SLAB_ATTR(store_user);
 
@@ -4156,16 +4152,13 @@ SLAB_ATTR_RO(free_calls);
 #ifdef CONFIG_FAILSLAB
 static ssize_t failslab_show(struct kmem_cache *s, char *buf)
 {
-	return sprintf(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB));
+	return flag_show(s, buf, SLAB_FAILSLAB);
 }
 
 static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
 							size_t length)
 {
-	s->flags &= ~SLAB_FAILSLAB;
-	if (buf[0] == '1')
-		s->flags |= SLAB_FAILSLAB;
-	return length;
+	return flag_store(s, buf, length, SLAB_FAILSLAB);
 }
 SLAB_ATTR(failslab);
 #endif
-- 
1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[Matt Mackall]

On Mon, 2011-03-14 at 21:58 -0400, George Spelvin wrote:
> For sysfs files that map a boolean to a flags bit.

This one's actually pretty nice. You should really try to put all the
uncontroversial bits of a series first.

> ---
>  mm/slub.c |   93 ++++++++++++++++++++++++++++--------------------------------
>  1 files changed, 43 insertions(+), 50 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index e15aa7f..856246f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3982,38 +3982,61 @@ static ssize_t objects_partial_show(struct kmem_cache *s, char *buf)
>  }
>  SLAB_ATTR_RO(objects_partial);
>  
> +static ssize_t flag_show(struct kmem_cache *s, char *buf, unsigned flag)
> +{
> +	return sprintf(buf, "%d\n", !!(s->flags & flag));
> +}
> +
> +static ssize_t flag_store(struct kmem_cache *s,
> +				const char *buf, size_t length, unsigned flag)
> +{
> +	s->flags &= ~flag;
> +	if (buf[0] == '1')
> +		s->flags |= flag;
> +	return length;
> +}
> +
> +/* Like above, but changes allocation size; so only allowed on empty slab */
> +static ssize_t flag_store_sizechange(struct kmem_cache *s,
> +				const char *buf, size_t length, unsigned flag)
> +{
> +	if (any_slab_objects(s))
> +		return -EBUSY;
> +
> +	flag_store(s, buf, length, flag);
> +	calculate_sizes(s, -1);
> +	return length;
> +}
> +
>  static ssize_t reclaim_account_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_RECLAIM_ACCOUNT));
> +	return flag_show(s, buf, SLAB_RECLAIM_ACCOUNT);
>  }
>  
>  static ssize_t reclaim_account_store(struct kmem_cache *s,
>  				const char *buf, size_t length)
>  {
> -	s->flags &= ~SLAB_RECLAIM_ACCOUNT;
> -	if (buf[0] == '1')
> -		s->flags |= SLAB_RECLAIM_ACCOUNT;
> -	return length;
> +	return flag_store(s, buf, length, SLAB_RECLAIM_ACCOUNT);
>  }
>  SLAB_ATTR(reclaim_account);
>  
>  static ssize_t hwcache_align_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_HWCACHE_ALIGN));
> +	return flag_show(s, buf, SLAB_HWCACHE_ALIGN);
>  }
>  SLAB_ATTR_RO(hwcache_align);
>  
>  #ifdef CONFIG_ZONE_DMA
>  static ssize_t cache_dma_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_CACHE_DMA));
> +	return flag_show(s, buf, SLAB_CACHE_DMA);
>  }
>  SLAB_ATTR_RO(cache_dma);
>  #endif
>  
>  static ssize_t destroy_by_rcu_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_DESTROY_BY_RCU));
> +	return flag_show(s, buf, SLAB_DESTROY_BY_RCU);
>  }
>  SLAB_ATTR_RO(destroy_by_rcu);
>  
> @@ -4032,88 +4055,61 @@ SLAB_ATTR_RO(total_objects);
>  
>  static ssize_t sanity_checks_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_DEBUG_FREE));
> +	return flag_show(s, buf, SLAB_DEBUG_FREE);
>  }
>  
>  static ssize_t sanity_checks_store(struct kmem_cache *s,
>  				const char *buf, size_t length)
>  {
> -	s->flags &= ~SLAB_DEBUG_FREE;
> -	if (buf[0] == '1')
> -		s->flags |= SLAB_DEBUG_FREE;
> -	return length;
> +	return flag_store(s, buf, length, SLAB_DEBUG_FREE);
>  }
>  SLAB_ATTR(sanity_checks);
>  
>  static ssize_t trace_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_TRACE));
> +	return flag_show(s, buf, SLAB_TRACE);
>  }
>  
>  static ssize_t trace_store(struct kmem_cache *s, const char *buf,
>  							size_t length)
>  {
> -	s->flags &= ~SLAB_TRACE;
> -	if (buf[0] == '1')
> -		s->flags |= SLAB_TRACE;
> -	return length;
> +	return flag_store(s, buf, length, SLAB_TRACE);
>  }
>  SLAB_ATTR(trace);
>  
>  static ssize_t red_zone_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_RED_ZONE));
> +	return flag_show(s, buf, SLAB_RED_ZONE);
>  }
>  
>  static ssize_t red_zone_store(struct kmem_cache *s,
>  				const char *buf, size_t length)
>  {
> -	if (any_slab_objects(s))
> -		return -EBUSY;
> -
> -	s->flags &= ~SLAB_RED_ZONE;
> -	if (buf[0] == '1')
> -		s->flags |= SLAB_RED_ZONE;
> -	calculate_sizes(s, -1);
> -	return length;
> +	return flag_store_sizechange(s, buf, length, SLAB_RED_ZONE);
>  }
>  SLAB_ATTR(red_zone);
>  
>  static ssize_t poison_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_POISON));
> +	return flag_show(s, buf, SLAB_POISON);
>  }
>  
>  static ssize_t poison_store(struct kmem_cache *s,
>  				const char *buf, size_t length)
>  {
> -	if (any_slab_objects(s))
> -		return -EBUSY;
> -
> -	s->flags &= ~SLAB_POISON;
> -	if (buf[0] == '1')
> -		s->flags |= SLAB_POISON;
> -	calculate_sizes(s, -1);
> -	return length;
> +	return flag_store_sizechange(s, buf, length, SLAB_POISON);
>  }
>  SLAB_ATTR(poison);
>  
>  static ssize_t store_user_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_STORE_USER));
> +	return flag_show(s, buf, SLAB_STORE_USER);
>  }
>  
>  static ssize_t store_user_store(struct kmem_cache *s,
>  				const char *buf, size_t length)
>  {
> -	if (any_slab_objects(s))
> -		return -EBUSY;
> -
> -	s->flags &= ~SLAB_STORE_USER;
> -	if (buf[0] == '1')
> -		s->flags |= SLAB_STORE_USER;
> -	calculate_sizes(s, -1);
> -	return length;
> +	return flag_store_sizechange(s, buf, length, SLAB_STORE_USER);
>  }
>  SLAB_ATTR(store_user);
>  
> @@ -4156,16 +4152,13 @@ SLAB_ATTR_RO(free_calls);
>  #ifdef CONFIG_FAILSLAB
>  static ssize_t failslab_show(struct kmem_cache *s, char *buf)
>  {
> -	return sprintf(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB));
> +	return flag_show(s, buf, SLAB_FAILSLAB);
>  }
>  
>  static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
>  							size_t length)
>  {
> -	s->flags &= ~SLAB_FAILSLAB;
> -	if (buf[0] == '1')
> -		s->flags |= SLAB_FAILSLAB;
> -	return length;
> +	return flag_store(s, buf, length, SLAB_FAILSLAB);
>  }
>  SLAB_ATTR(failslab);
>  #endif


-- 
Mathematics is the supreme nostalgia of our time.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[Pekka Enberg]

On Tue, Mar 15, 2011 at 3:58 AM, George Spelvin <linux@horizon.com> wrote:
> For sysfs files that map a boolean to a flags bit.

Looks good to me. I'll cc Christoph and David too.

> ---
>  mm/slub.c |   93 ++++++++++++++++++++++++++++--------------------------------
>  1 files changed, 43 insertions(+), 50 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index e15aa7f..856246f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3982,38 +3982,61 @@ static ssize_t objects_partial_show(struct kmem_cache *s, char *buf)
>  }
>  SLAB_ATTR_RO(objects_partial);
>
> +static ssize_t flag_show(struct kmem_cache *s, char *buf, unsigned flag)
> +{
> +       return sprintf(buf, "%d\n", !!(s->flags & flag));
> +}
> +
> +static ssize_t flag_store(struct kmem_cache *s,
> +                               const char *buf, size_t length, unsigned flag)
> +{
> +       s->flags &= ~flag;
> +       if (buf[0] == '1')
> +               s->flags |= flag;
> +       return length;
> +}
> +
> +/* Like above, but changes allocation size; so only allowed on empty slab */
> +static ssize_t flag_store_sizechange(struct kmem_cache *s,
> +                               const char *buf, size_t length, unsigned flag)
> +{
> +       if (any_slab_objects(s))
> +               return -EBUSY;
> +
> +       flag_store(s, buf, length, flag);
> +       calculate_sizes(s, -1);
> +       return length;
> +}
> +
>  static ssize_t reclaim_account_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_RECLAIM_ACCOUNT));
> +       return flag_show(s, buf, SLAB_RECLAIM_ACCOUNT);
>  }
>
>  static ssize_t reclaim_account_store(struct kmem_cache *s,
>                                const char *buf, size_t length)
>  {
> -       s->flags &= ~SLAB_RECLAIM_ACCOUNT;
> -       if (buf[0] == '1')
> -               s->flags |= SLAB_RECLAIM_ACCOUNT;
> -       return length;
> +       return flag_store(s, buf, length, SLAB_RECLAIM_ACCOUNT);
>  }
>  SLAB_ATTR(reclaim_account);
>
>  static ssize_t hwcache_align_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_HWCACHE_ALIGN));
> +       return flag_show(s, buf, SLAB_HWCACHE_ALIGN);
>  }
>  SLAB_ATTR_RO(hwcache_align);
>
>  #ifdef CONFIG_ZONE_DMA
>  static ssize_t cache_dma_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_CACHE_DMA));
> +       return flag_show(s, buf, SLAB_CACHE_DMA);
>  }
>  SLAB_ATTR_RO(cache_dma);
>  #endif
>
>  static ssize_t destroy_by_rcu_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_DESTROY_BY_RCU));
> +       return flag_show(s, buf, SLAB_DESTROY_BY_RCU);
>  }
>  SLAB_ATTR_RO(destroy_by_rcu);
>
> @@ -4032,88 +4055,61 @@ SLAB_ATTR_RO(total_objects);
>
>  static ssize_t sanity_checks_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_DEBUG_FREE));
> +       return flag_show(s, buf, SLAB_DEBUG_FREE);
>  }
>
>  static ssize_t sanity_checks_store(struct kmem_cache *s,
>                                const char *buf, size_t length)
>  {
> -       s->flags &= ~SLAB_DEBUG_FREE;
> -       if (buf[0] == '1')
> -               s->flags |= SLAB_DEBUG_FREE;
> -       return length;
> +       return flag_store(s, buf, length, SLAB_DEBUG_FREE);
>  }
>  SLAB_ATTR(sanity_checks);
>
>  static ssize_t trace_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_TRACE));
> +       return flag_show(s, buf, SLAB_TRACE);
>  }
>
>  static ssize_t trace_store(struct kmem_cache *s, const char *buf,
>                                                        size_t length)
>  {
> -       s->flags &= ~SLAB_TRACE;
> -       if (buf[0] == '1')
> -               s->flags |= SLAB_TRACE;
> -       return length;
> +       return flag_store(s, buf, length, SLAB_TRACE);
>  }
>  SLAB_ATTR(trace);
>
>  static ssize_t red_zone_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_RED_ZONE));
> +       return flag_show(s, buf, SLAB_RED_ZONE);
>  }
>
>  static ssize_t red_zone_store(struct kmem_cache *s,
>                                const char *buf, size_t length)
>  {
> -       if (any_slab_objects(s))
> -               return -EBUSY;
> -
> -       s->flags &= ~SLAB_RED_ZONE;
> -       if (buf[0] == '1')
> -               s->flags |= SLAB_RED_ZONE;
> -       calculate_sizes(s, -1);
> -       return length;
> +       return flag_store_sizechange(s, buf, length, SLAB_RED_ZONE);
>  }
>  SLAB_ATTR(red_zone);
>
>  static ssize_t poison_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_POISON));
> +       return flag_show(s, buf, SLAB_POISON);
>  }
>
>  static ssize_t poison_store(struct kmem_cache *s,
>                                const char *buf, size_t length)
>  {
> -       if (any_slab_objects(s))
> -               return -EBUSY;
> -
> -       s->flags &= ~SLAB_POISON;
> -       if (buf[0] == '1')
> -               s->flags |= SLAB_POISON;
> -       calculate_sizes(s, -1);
> -       return length;
> +       return flag_store_sizechange(s, buf, length, SLAB_POISON);
>  }
>  SLAB_ATTR(poison);
>
>  static ssize_t store_user_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_STORE_USER));
> +       return flag_show(s, buf, SLAB_STORE_USER);
>  }
>
>  static ssize_t store_user_store(struct kmem_cache *s,
>                                const char *buf, size_t length)
>  {
> -       if (any_slab_objects(s))
> -               return -EBUSY;
> -
> -       s->flags &= ~SLAB_STORE_USER;
> -       if (buf[0] == '1')
> -               s->flags |= SLAB_STORE_USER;
> -       calculate_sizes(s, -1);
> -       return length;
> +       return flag_store_sizechange(s, buf, length, SLAB_STORE_USER);
>  }
>  SLAB_ATTR(store_user);
>
> @@ -4156,16 +4152,13 @@ SLAB_ATTR_RO(free_calls);
>  #ifdef CONFIG_FAILSLAB
>  static ssize_t failslab_show(struct kmem_cache *s, char *buf)
>  {
> -       return sprintf(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB));
> +       return flag_show(s, buf, SLAB_FAILSLAB);
>  }
>
>  static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
>                                                        size_t length)
>  {
> -       s->flags &= ~SLAB_FAILSLAB;
> -       if (buf[0] == '1')
> -               s->flags |= SLAB_FAILSLAB;
> -       return length;
> +       return flag_store(s, buf, length, SLAB_FAILSLAB);
>  }
>  SLAB_ATTR(failslab);
>  #endif
> --
> 1.7.4.1
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[David Rientjes]

On Mon, 14 Mar 2011, George Spelvin wrote:

> For sysfs files that map a boolean to a flags bit.

Where's your signed-off-by?

> ---
>  mm/slub.c |   93 ++++++++++++++++++++++++++++--------------------------------
>  1 files changed, 43 insertions(+), 50 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index e15aa7f..856246f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3982,38 +3982,61 @@ static ssize_t objects_partial_show(struct kmem_cache *s, char *buf)
>  }
>  SLAB_ATTR_RO(objects_partial);
>  
> +static ssize_t flag_show(struct kmem_cache *s, char *buf, unsigned flag)
> +{
> +	return sprintf(buf, "%d\n", !!(s->flags & flag));
> +}
> +
> +static ssize_t flag_store(struct kmem_cache *s,
> +				const char *buf, size_t length, unsigned flag)
> +{
> +	s->flags &= ~flag;
> +	if (buf[0] == '1')
> +		s->flags |= flag;
> +	return length;
> +}
> +
> +/* Like above, but changes allocation size; so only allowed on empty slab */
> +static ssize_t flag_store_sizechange(struct kmem_cache *s,
> +				const char *buf, size_t length, unsigned flag)
> +{
> +	if (any_slab_objects(s))
> +		return -EBUSY;
> +
> +	flag_store(s, buf, length, flag);
> +	calculate_sizes(s, -1);
> +	return length;
> +}
> +

Nice cleanup.

"flag" should be unsigned long in all of these functions: the constants 
are declared with UL suffixes in slab.h.

After that's fixed,

	Acked-by: David Rientjes <rientjes@google.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[George Spelvin]

> Where's your signed-off-by?

Somewhere under the pile of crap on my desk. :-)
(More to the point, waiting for me to think it's good enough to submit
For Real.)

> Nice cleanup.
> 
> "flag" should be unsigned long in all of these functions: the constants 
> are declared with UL suffixes in slab.h.

Actually, I did that deliberately.  Because there's a problem I keep
wondering about, which repeats many many times in the kernel:

*Why* are they unsigned long?  That's an awkward type: 32 bits on many
architectures, so we can't portably assign more than 32 bits, and on
platforms where it's 64 bits, the upper 32 are just wasting space.
(And REX prefixes on x86-64.)

Wouldn't it be a better cleanup to convert the whole lot to unsigned
or u32?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[David Rientjes]

On Wed, 16 Mar 2011, George Spelvin wrote:

> > Where's your signed-off-by?
> 
> Somewhere under the pile of crap on my desk. :-)
> (More to the point, waiting for me to think it's good enough to submit
> For Real.)
> 

Patches that you would like to propose but don't think are ready for merge 
should have s/PATCH/RFC/ done on the subject line.

> > Nice cleanup.
> > 
> > "flag" should be unsigned long in all of these functions: the constants 
> > are declared with UL suffixes in slab.h.
> 
> Actually, I did that deliberately.  Because there's a problem I keep
> wondering about, which repeats many many times in the kernel:
> 

You deliberately created a helper function to take an unsigned int when 
the actuals being passed in are all unsigned long to trigger a discussion 
on why they are unsigned long?

> *Why* are they unsigned long?  That's an awkward type: 32 bits on many
> architectures, so we can't portably assign more than 32 bits, and on
> platforms where it's 64 bits, the upper 32 are just wasting space.
> (And REX prefixes on x86-64.)
> 

unsigned long uses the native word size of the architecture which can 
generate more efficient code; we typically imply that flags have a limited 
size by including leading zeros in their definition for 32-bit 
compatibility:

#define SLAB_DEBUG_FREE         0x00000100UL    /* DEBUG: Perform (expensive) checks on free */
#define SLAB_RED_ZONE           0x00000400UL    /* DEBUG: Red zone objs in a cache */
#define SLAB_POISON             0x00000800UL    /* DEBUG: Poison objects */
...

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[George Spelvin]

> Patches that you would like to propose but don't think are ready for merge 
> should have s/PATCH/RFC/ done on the subject line.

You're right; I should have.  I blame git-format-patch's defaults, but mea culpa.
(Now I know about the --subject-prefix=RFC option!)

> You deliberately created a helper function to take an unsigned int when 
> the actuals being passed in are all unsigned long to trigger a discussion 
> on why they are unsigned long?

Er, no, I'm not that Machiavellian.
I deliberately did it because it was obvious that the flags would always
fit into an "unsigned", so I didn't need "unsigned long".

(Actually, I owe you an apology; when writing that e-mail, I remember
thinking "I should go back and clarify that statement", but forgot before
hitting send.)

> unsigned long uses the native word size of the architecture which can 
> generate more efficient code; we typically imply that flags have a limited 
> size by including leading zeros in their definition for 32-bit 
> compatibility:

Um, can you name a (64-bit) architecture on which 32-bit is more
expensive than 64-bit?  On x86-64, it's potentially cheaper, and even
the infamous Alpha 21064 has no penalty for 32-bit accesses.  SPARC,
MIPS, PPC, Itanium, what else?  I don't know about z/ARchitecture,
but given the emphasis on backward compatibility in IBM's mainframes,
it seems hard to imagine.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[Pekka Enberg]

Hi George!

On Thu, Mar 17, 2011 at 12:36 AM, George Spelvin <linux@horizon.com> wrote:
> Um, can you name a (64-bit) architecture on which 32-bit is more
> expensive than 64-bit?

I certainly don't but I'd still like to ask you to change it to
'unsigned long'. That's a Linux kernel idiom and we're not going to
change the whole kernel.

                        Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[George Spelvin]

> I certainly don't but I'd still like to ask you to change it to
> 'unsigned long'. That's a Linux kernel idiom and we're not going to
> change the whole kernel.

Damn, and I just prepared the following patch.  Should I, instead, do

--- a/include/linux/slab_def.h
+++ b/include/linux/slab_def.h
@@ -62,5 +62,5 @@ struct kmem_cache {
 /* 3) touched by every alloc & free from the backend */
 
-	unsigned int flags;		/* constant flags */
+	unsigned long flags;		/* constant flags */
 	unsigned int num;		/* # of objs per slab */
 
... because the original slab code uses an unsigned int.  To fix it
the other way (for SLAB_ flags only) is a patch like this:

Re: [PATCH 5/8] mm/slub: Factor out some common code.

[Pekka Enberg]

On Thu, Mar 17, 2011 at 9:07 AM, George Spelvin <linux@horizon.com> wrote:
>> I certainly don't but I'd still like to ask you to change it to
>> 'unsigned long'. That's a Linux kernel idiom and we're not going to
>> change the whole kernel.
>
> Damn, and I just prepared the following patch.  Should I, instead, do
>
> --- a/include/linux/slab_def.h
> +++ b/include/linux/slab_def.h
> @@ -62,5 +62,5 @@ struct kmem_cache {
>  /* 3) touched by every alloc & free from the backend */
>
> -       unsigned int flags;             /* constant flags */
> +       unsigned long flags;            /* constant flags */
>        unsigned int num;               /* # of objs per slab */
>
> ... because the original slab code uses an unsigned int.

Looks good to me.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>