mm/slub.c: use 'unsigned long' instead of 'int' for variable 'slub_debug'

PATCH In review Chen Gang 4 participants 22 messages Jul 12, 2013 – Jul 22, 2013

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Chen Gang

author

2013-07-12 01:44 UTC #0 lore raw

PATCH 1/1 mm/slub.c: use 'unsigned long' instead of 'int' for variable 'slub_debug'

Since all values which can be assigned to 'slub_debug' are 'unsigned long', recommend also to define 'slub_debug' as 'unsigned long' to match the type precisely

e.g DEBUG_DEFAULT_FLAGS, SLAB_TRACE, SLAB_FAILSLAB ...

Signed-off-by Chen Gang <[email protected]>

---
 mm/slub.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/slub.c +2 -2

index 2b02d66..4fd8c50 100644
@@ -452,9 +452,9 @@ static void get_map(struct kmem_cache *s, struct page *page, unsigned long *map)
  * Debug settings:
  */
 #ifdef CONFIG_SLUB_DEBUG_ON
-static int slub_debug = DEBUG_DEFAULT_FLAGS;
+static unsigned long slub_debug = DEBUG_DEFAULT_FLAGS;
 #else
-static int slub_debug;
+static unsigned long slub_debug;
 #endif
 
 static char *slub_debug_slabs;

Signature · 7 lines

1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Chen Gang

author

2013-07-12 03:28 UTC #1 lore raw

PATCH 1/1 mm/slub.c: beautify code of this file

Be sure of 80 column limitation for both code and comments.

Correct tab alignment for 'if-else' statement.

Remove redundancy 'break' statement.

Remove useless BUG_ON(), since it can never happen.

Signed-off-by Chen Gang <[email protected]>

---
 mm/slub.c |   94 ++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 56 insertions(+), 38 deletions(-)

mm/slub.c +56 ~~-38~~

index 2b02d66..2b02d64 100644
@@ -373,7 +373,8 @@ static inline bool __cmpxchg_double_slab(struct kmem_cache *s, struct page *page
 #endif
 	{
 		slab_lock(page);
-		if (page->freelist == freelist_old && page->counters == counters_old) {
+		if (page->freelist == freelist_old &&
+					page->counters == counters_old) {
 			page->freelist = freelist_new;
 			page->counters = counters_new;
 			slab_unlock(page);

@@ -411,7 +412,8 @@ static inline bool cmpxchg_double_slab(struct kmem_cache *s, struct page *page,
 
 		local_irq_save(flags);
 		slab_lock(page);
-		if (page->freelist == freelist_old && page->counters == counters_old) {
+		if (page->freelist == freelist_old &&
+					page->counters == counters_old) {
 			page->freelist = freelist_new;
 			page->counters = counters_new;
 			slab_unlock(page);

@@ -553,8 +555,9 @@ static void print_tracking(struct kmem_cache *s, void *object)
 
 static void print_page_info(struct page *page)
 {
-	printk(KERN_ERR "INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=0x%04lx\n",
-		page, page->objects, page->inuse, page->freelist, page->flags);
+	printk(KERN_ERR
+	       "INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=0x%04lx\n",
+	       page, page->objects, page->inuse, page->freelist, page->flags);
 
 }

@@ -629,7 +632,8 @@ static void object_err(struct kmem_cache *s, struct page *page,
 	print_trailer(s, page, object);
 }
 
-static void slab_err(struct kmem_cache *s, struct page *page, const char *fmt, ...)
+static void slab_err(struct kmem_cache *s, struct page *page,
+			const char *fmt, ...)
 {
 	va_list args;
 	char buf[100];

@@ -788,7 +792,8 @@ static int check_object(struct kmem_cache *s, struct page *page,
 	} else {
 		if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) {
 			check_bytes_and_report(s, page, p, "Alignment padding",
-				endobject, POISON_INUSE, s->inuse - s->object_size);
+				endobject, POISON_INUSE,
+				s->inuse - s->object_size);
 		}
 	}

@@ -881,7 +886,6 @@ static int on_freelist(struct kmem_cache *s, struct page *page, void *search)
 				slab_fix(s, "Freelist cleared");
 				return 0;
 			}
-			break;
 		}
 		object = fp;
 		fp = get_freepointer(s, object);

@@ -918,7 +922,8 @@ static void trace(struct kmem_cache *s, struct page *page, void *object,
 			page->freelist);
 
 		if (!alloc)
-			print_section("Object ", (void *)object, s->object_size);
+			print_section("Object ", (void *)object,
+					s->object_size);
 
 		dump_stack();
 	}

@@ -937,7 +942,8 @@ static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
 	return should_failslab(s->object_size, flags, s->flags);
 }
 
-static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags, void *object)
+static inline void slab_post_alloc_hook(struct kmem_cache *s,
+					gfp_t flags, void *object)
 {
 	flags &= gfp_allowed_mask;
 	kmemcheck_slab_alloc(s, flags, object, slab_ksize(s));

@@ -1039,7 +1045,8 @@ static void setup_object_debug(struct kmem_cache *s, struct page *page,
 	init_tracking(s, object);
 }
 
-static noinline int alloc_debug_processing(struct kmem_cache *s, struct page *page,
+static noinline int alloc_debug_processing(struct kmem_cache *s,
+					struct page *page,
 					void *object, unsigned long addr)
 {
 	if (!check_slab(s, page))

@@ -1743,7 +1750,8 @@ static void init_kmem_cache_cpus(struct kmem_cache *s)
 /*
  * Remove the cpu slab
  */
-static void deactivate_slab(struct kmem_cache *s, struct page *page, void *freelist)
+static void deactivate_slab(struct kmem_cache *s, struct page *page,
+				void *freelist)
 {
 	enum slab_modes { M_NONE, M_PARTIAL, M_FULL, M_FREE };
 	struct kmem_cache_node *n = get_node(s, page_to_nid(page));

@@ -2002,7 +2010,8 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 		page->pobjects = pobjects;
 		page->next = oldpage;
 
-	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page) != oldpage);
+	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page)
+								!= oldpage);
 #endif
 }

@@ -2172,8 +2181,8 @@ static inline bool pfmemalloc_match(struct page *page, gfp_t gfpflags)
 }
 
 /*
- * Check the page->freelist of a page and either transfer the freelist to the per cpu freelist
- * or deactivate the page.
+ * Check the page->freelist of a page and either transfer the freelist to the
+ * per cpu freelist or deactivate the page.
  *
  * The page is still frozen if the return value is not NULL.
  *

@@ -2317,7 +2326,8 @@ new_slab:
 		goto load_freelist;
 
 	/* Only entered in the debug case */
-	if (kmem_cache_debug(s) && !alloc_debug_processing(s, page, freelist, addr))
+	if (kmem_cache_debug(s) &&
+			!alloc_debug_processing(s, page, freelist, addr))
 		goto new_slab;	/* Slab failed checks. Next slab needed */
 
 	deactivate_slab(s, page, get_freepointer(s, freelist));

@@ -2385,13 +2395,15 @@ redo:
 		 * The cmpxchg will only match if there was no additional
 		 * operation and if we are on the right processor.
 		 *
-		 * The cmpxchg does the following atomically (without lock semantics!)
+		 * The cmpxchg does the following atomically (without lock
+		 * semantics!)
 		 * 1. Relocate first pointer to the current per cpu area.
 		 * 2. Verify that tid and freelist have not been changed
 		 * 3. If they were not changed replace tid and freelist
 		 *
-		 * Since this is without lock semantics the protection is only against
-		 * code executing on this cpu *not* from access by other cpus.
+		 * Since this is without lock semantics the protection is only
+		 * against code executing on this cpu *not* from access by
+		 * other cpus.
 		 */
 		if (unlikely(!this_cpu_cmpxchg_double(
 				s->cpu_slab->freelist, s->cpu_slab->tid,

@@ -2423,7 +2435,8 @@ void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
 {
 	void *ret = slab_alloc(s, gfpflags, _RET_IP_);
 
-	trace_kmem_cache_alloc(_RET_IP_, ret, s->object_size, s->size, gfpflags);
+	trace_kmem_cache_alloc(_RET_IP_, ret, s->object_size,
+				s->size, gfpflags);
 
 	return ret;
 }

@@ -2515,8 +2528,10 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
 			if (kmem_cache_has_cpu_partial(s) && !prior)
 
 				/*
-				 * Slab was on no list before and will be partially empty
-				 * We can defer the list move and instead freeze it.
+				 * Slab was on no list before and will be
+				 * partially empty
+				 * We can defer the list move and instead
+				 * freeze it.
 				 */
 				new.frozen = 1;

@@ -3074,8 +3089,8 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags)
 	 * A) The number of objects from per cpu partial slabs dumped to the
 	 *    per node list when we reach the limit.
 	 * B) The number of objects in cpu partial slabs to extract from the
-	 *    per node list when we run out of per cpu objects. We only fetch 50%
-	 *    to keep some capacity around for frees.
+	 *    per node list when we run out of per cpu objects. We only fetch
+	 *    50% to keep some capacity around for frees.
 	 */
 	if (!kmem_cache_has_cpu_partial(s))
 		s->cpu_partial = 0;

@@ -3102,8 +3117,8 @@ error:
 	if (flags & SLAB_PANIC)
 		panic("Cannot create slab %s size=%lu realsize=%u "
 			"order=%u offset=%u flags=%lx\n",
-			s->name, (unsigned long)s->size, s->size, oo_order(s->oo),
-			s->offset, flags);
+			s->name, (unsigned long)s->size, s->size,
+			oo_order(s->oo), s->offset, flags);
 	return -EINVAL;
 }

@@ -3341,7 +3356,8 @@ bool verify_mem_not_deleted(const void *x)
 
 	slab_lock(page);
 	if (on_freelist(page->slab_cache, page, object)) {
-		object_err(page->slab_cache, page, object, "Object is on free-list");
+		object_err(page->slab_cache, page, object,
+				"Object is on free-list");
 		rv = false;
 	} else {
 		rv = true;

@@ -4165,15 +4181,17 @@ static int list_locations(struct kmem_cache *s, char *buf,
 				!cpumask_empty(to_cpumask(l->cpus)) &&
 				len < PAGE_SIZE - 60) {
 			len += sprintf(buf + len, " cpus=");
-			len += cpulist_scnprintf(buf + len, PAGE_SIZE - len - 50,
+			len += cpulist_scnprintf(buf + len,
+						 PAGE_SIZE - len - 50,
 						 to_cpumask(l->cpus));
 		}
 
 		if (nr_online_nodes > 1 && !nodes_empty(l->nodes) &&
 				len < PAGE_SIZE - 60) {
 			len += sprintf(buf + len, " nodes=");
-			len += nodelist_scnprintf(buf + len, PAGE_SIZE - len - 50,
-					l->nodes);
+			len += nodelist_scnprintf(buf + len,
+						  PAGE_SIZE - len - 50,
+						  l->nodes);
 		}
 
 		len += sprintf(buf + len, "\n");

@@ -4282,7 +4300,8 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 		int cpu;
 
 		for_each_possible_cpu(cpu) {
-			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
+			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab,
+							       cpu);
 			int node;
 			struct page *page;

@@ -4318,12 +4337,11 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 		for_each_node_state(node, N_NORMAL_MEMORY) {
 			struct kmem_cache_node *n = get_node(s, node);
 
-		if (flags & SO_TOTAL)
-			x = atomic_long_read(&n->total_objects);
-		else if (flags & SO_OBJECTS)
-			x = atomic_long_read(&n->total_objects) -
-				count_partial(n, count_free);
-
+			if (flags & SO_TOTAL)
+				x = atomic_long_read(&n->total_objects);
+			else if (flags & SO_OBJECTS)
+				x = atomic_long_read(&n->total_objects) -
+					count_partial(n, count_free);
 			else
 				x = atomic_long_read(&n->nr_slabs);
 			total += x;

@@ -5139,10 +5157,10 @@ static char *create_unique_id(struct kmem_cache *s)
 
 #ifdef CONFIG_MEMCG_KMEM
 	if (!is_root_cache(s))
-		p += sprintf(p, "-%08d", memcg_cache_id(s->memcg_params->memcg));
+		p += sprintf(p, "-%08d",
+				memcg_cache_id(s->memcg_params->memcg));
 #endif
 
-	BUG_ON(p > name + ID_STR_LENGTH - 1);
 	return name;
 }

Signature · 7 lines

1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-07-12 13:58 UTC #2 lore raw

Re: [PATCH] mm/slub.c: beautify code of this file

Quoted 2 lines · Chen Gang

Be sure of 80 column limitation for both code and comments. Correct tab alignment for 'if-else' statement.

Thanks.

Quoted 1 line

Remove redundancy 'break' statement.

Hmm... I'd rather have the first break removed.

Quoted 1 line

Remove useless BUG_ON(), since it can never happen.

It may happen if more code is added to that function. Recently the cgroups thing was added f.e.

Could you separate this out into multiple patches that each do one thing only?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Chen Gang

author

2013-07-15 00:32 UTC #3 lore raw

Re: [PATCH] mm/slub.c: beautify code of this file

Quoted 7 lines · Christoph Lameter

Quoted 2 lines · Chen Gang
Be sure of 80 column limitation for both code and comments. Correct tab alignment for 'if-else' statement.
Thanks.

Thank you too.

Quoted 4 lines

Quoted 1 line
Remove redundancy 'break' statement.
Hmm... I'd rather have the first break removed.

Yeah, that will be better for coding and understanding. I will send patch v2 for it.

Quoted 5 lines

Quoted 1 line
Remove useless BUG_ON(), since it can never happen.
It may happen if more code is added to that function. Recently the cgroups thing was added f.e.

Hmm... at least what you said is reasonable. I need respect the related maintainers' willing and opinions (e.g. you for "slub.c").

Quoted 3 lines

Could you separate this out into multiple patches that each do one thing only?

OK, thanks. I will do.

Originally, I think these are minor, I should do with them in a patch, so can save maintainers' time resource (now, I know it is incorrect).

:-)

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Chen Gang

author

2013-07-15 01:05 UTC #4 lore raw

cover mm/slub.c: beautify code of this file

In reply to Christoph Lameter

Beautify code for 80 column limitation and tab alignment (patch 1/2).

Also remove redundancy 'break' statement (patch 2/2).

---
 mm/slub.c |   93 ++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 56 insertions(+), 37 deletions(-)

Chen Gang

author

2013-07-15 01:06 UTC #5 lore raw

PATCH 1/2 mm/slub.c: beautify code for 80 column limitation and tab alignment.

In reply to Christoph Lameter

Be sure of 80 column limitation for both code and comments.

Correct tab alignment for 'if-else' statement.

Signed-off-by Chen Gang <[email protected]>

---
 mm/slub.c |   92 +++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 56 insertions(+), 36 deletions(-)

mm/slub.c +56 ~~-36~~

index 2b02d66..05ab2d5 100644
@@ -373,7 +373,8 @@ static inline bool __cmpxchg_double_slab(struct kmem_cache *s, struct page *page
 #endif
 	{
 		slab_lock(page);
-		if (page->freelist == freelist_old && page->counters == counters_old) {
+		if (page->freelist == freelist_old &&
+					page->counters == counters_old) {
 			page->freelist = freelist_new;
 			page->counters = counters_new;
 			slab_unlock(page);

@@ -411,7 +412,8 @@ static inline bool cmpxchg_double_slab(struct kmem_cache *s, struct page *page,
 
 		local_irq_save(flags);
 		slab_lock(page);
-		if (page->freelist == freelist_old && page->counters == counters_old) {
+		if (page->freelist == freelist_old &&
+					page->counters == counters_old) {
 			page->freelist = freelist_new;
 			page->counters = counters_new;
 			slab_unlock(page);

@@ -553,8 +555,9 @@ static void print_tracking(struct kmem_cache *s, void *object)
 
 static void print_page_info(struct page *page)
 {
-	printk(KERN_ERR "INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=0x%04lx\n",
-		page, page->objects, page->inuse, page->freelist, page->flags);
+	printk(KERN_ERR
+	       "INFO: Slab 0x%p objects=%u used=%u fp=0x%p flags=0x%04lx\n",
+	       page, page->objects, page->inuse, page->freelist, page->flags);
 
 }

@@ -629,7 +632,8 @@ static void object_err(struct kmem_cache *s, struct page *page,
 	print_trailer(s, page, object);
 }
 
-static void slab_err(struct kmem_cache *s, struct page *page, const char *fmt, ...)
+static void slab_err(struct kmem_cache *s, struct page *page,
+			const char *fmt, ...)
 {
 	va_list args;
 	char buf[100];

@@ -788,7 +792,8 @@ static int check_object(struct kmem_cache *s, struct page *page,
 	} else {
 		if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) {
 			check_bytes_and_report(s, page, p, "Alignment padding",
-				endobject, POISON_INUSE, s->inuse - s->object_size);
+				endobject, POISON_INUSE,
+				s->inuse - s->object_size);
 		}
 	}

@@ -918,7 +923,8 @@ static void trace(struct kmem_cache *s, struct page *page, void *object,
 			page->freelist);
 
 		if (!alloc)
-			print_section("Object ", (void *)object, s->object_size);
+			print_section("Object ", (void *)object,
+					s->object_size);
 
 		dump_stack();
 	}

@@ -937,7 +943,8 @@ static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
 	return should_failslab(s->object_size, flags, s->flags);
 }
 
-static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags, void *object)
+static inline void slab_post_alloc_hook(struct kmem_cache *s,
+					gfp_t flags, void *object)
 {
 	flags &= gfp_allowed_mask;
 	kmemcheck_slab_alloc(s, flags, object, slab_ksize(s));

@@ -1039,7 +1046,8 @@ static void setup_object_debug(struct kmem_cache *s, struct page *page,
 	init_tracking(s, object);
 }
 
-static noinline int alloc_debug_processing(struct kmem_cache *s, struct page *page,
+static noinline int alloc_debug_processing(struct kmem_cache *s,
+					struct page *page,
 					void *object, unsigned long addr)
 {
 	if (!check_slab(s, page))

@@ -1743,7 +1751,8 @@ static void init_kmem_cache_cpus(struct kmem_cache *s)
 /*
  * Remove the cpu slab
  */
-static void deactivate_slab(struct kmem_cache *s, struct page *page, void *freelist)
+static void deactivate_slab(struct kmem_cache *s, struct page *page,
+				void *freelist)
 {
 	enum slab_modes { M_NONE, M_PARTIAL, M_FULL, M_FREE };
 	struct kmem_cache_node *n = get_node(s, page_to_nid(page));

@@ -2002,7 +2011,8 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 		page->pobjects = pobjects;
 		page->next = oldpage;
 
-	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page) != oldpage);
+	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page)
+								!= oldpage);
 #endif
 }

@@ -2172,8 +2182,8 @@ static inline bool pfmemalloc_match(struct page *page, gfp_t gfpflags)
 }
 
 /*
- * Check the page->freelist of a page and either transfer the freelist to the per cpu freelist
- * or deactivate the page.
+ * Check the page->freelist of a page and either transfer the freelist to the
+ * per cpu freelist or deactivate the page.
  *
  * The page is still frozen if the return value is not NULL.
  *

@@ -2317,7 +2327,8 @@ new_slab:
 		goto load_freelist;
 
 	/* Only entered in the debug case */
-	if (kmem_cache_debug(s) && !alloc_debug_processing(s, page, freelist, addr))
+	if (kmem_cache_debug(s) &&
+			!alloc_debug_processing(s, page, freelist, addr))
 		goto new_slab;	/* Slab failed checks. Next slab needed */
 
 	deactivate_slab(s, page, get_freepointer(s, freelist));

@@ -2385,13 +2396,15 @@ redo:
 		 * The cmpxchg will only match if there was no additional
 		 * operation and if we are on the right processor.
 		 *
-		 * The cmpxchg does the following atomically (without lock semantics!)
+		 * The cmpxchg does the following atomically (without lock
+		 * semantics!)
 		 * 1. Relocate first pointer to the current per cpu area.
 		 * 2. Verify that tid and freelist have not been changed
 		 * 3. If they were not changed replace tid and freelist
 		 *
-		 * Since this is without lock semantics the protection is only against
-		 * code executing on this cpu *not* from access by other cpus.
+		 * Since this is without lock semantics the protection is only
+		 * against code executing on this cpu *not* from access by
+		 * other cpus.
 		 */
 		if (unlikely(!this_cpu_cmpxchg_double(
 				s->cpu_slab->freelist, s->cpu_slab->tid,

@@ -2423,7 +2436,8 @@ void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
 {
 	void *ret = slab_alloc(s, gfpflags, _RET_IP_);
 
-	trace_kmem_cache_alloc(_RET_IP_, ret, s->object_size, s->size, gfpflags);
+	trace_kmem_cache_alloc(_RET_IP_, ret, s->object_size,
+				s->size, gfpflags);
 
 	return ret;
 }

@@ -2515,8 +2529,10 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
 			if (kmem_cache_has_cpu_partial(s) && !prior)
 
 				/*
-				 * Slab was on no list before and will be partially empty
-				 * We can defer the list move and instead freeze it.
+				 * Slab was on no list before and will be
+				 * partially empty
+				 * We can defer the list move and instead
+				 * freeze it.
 				 */
 				new.frozen = 1;

@@ -3074,8 +3090,8 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags)
 	 * A) The number of objects from per cpu partial slabs dumped to the
 	 *    per node list when we reach the limit.
 	 * B) The number of objects in cpu partial slabs to extract from the
-	 *    per node list when we run out of per cpu objects. We only fetch 50%
-	 *    to keep some capacity around for frees.
+	 *    per node list when we run out of per cpu objects. We only fetch
+	 *    50% to keep some capacity around for frees.
 	 */
 	if (!kmem_cache_has_cpu_partial(s))
 		s->cpu_partial = 0;

@@ -3102,8 +3118,8 @@ error:
 	if (flags & SLAB_PANIC)
 		panic("Cannot create slab %s size=%lu realsize=%u "
 			"order=%u offset=%u flags=%lx\n",
-			s->name, (unsigned long)s->size, s->size, oo_order(s->oo),
-			s->offset, flags);
+			s->name, (unsigned long)s->size, s->size,
+			oo_order(s->oo), s->offset, flags);
 	return -EINVAL;
 }

@@ -3341,7 +3357,8 @@ bool verify_mem_not_deleted(const void *x)
 
 	slab_lock(page);
 	if (on_freelist(page->slab_cache, page, object)) {
-		object_err(page->slab_cache, page, object, "Object is on free-list");
+		object_err(page->slab_cache, page, object,
+				"Object is on free-list");
 		rv = false;
 	} else {
 		rv = true;

@@ -4165,15 +4182,17 @@ static int list_locations(struct kmem_cache *s, char *buf,
 				!cpumask_empty(to_cpumask(l->cpus)) &&
 				len < PAGE_SIZE - 60) {
 			len += sprintf(buf + len, " cpus=");
-			len += cpulist_scnprintf(buf + len, PAGE_SIZE - len - 50,
+			len += cpulist_scnprintf(buf + len,
+						 PAGE_SIZE - len - 50,
 						 to_cpumask(l->cpus));
 		}
 
 		if (nr_online_nodes > 1 && !nodes_empty(l->nodes) &&
 				len < PAGE_SIZE - 60) {
 			len += sprintf(buf + len, " nodes=");
-			len += nodelist_scnprintf(buf + len, PAGE_SIZE - len - 50,
-					l->nodes);
+			len += nodelist_scnprintf(buf + len,
+						  PAGE_SIZE - len - 50,
+						  l->nodes);
 		}
 
 		len += sprintf(buf + len, "\n");

@@ -4282,7 +4301,8 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 		int cpu;
 
 		for_each_possible_cpu(cpu) {
-			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
+			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab,
+							       cpu);
 			int node;
 			struct page *page;

@@ -4318,12 +4338,11 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 		for_each_node_state(node, N_NORMAL_MEMORY) {
 			struct kmem_cache_node *n = get_node(s, node);
 
-		if (flags & SO_TOTAL)
-			x = atomic_long_read(&n->total_objects);
-		else if (flags & SO_OBJECTS)
-			x = atomic_long_read(&n->total_objects) -
-				count_partial(n, count_free);
-
+			if (flags & SO_TOTAL)
+				x = atomic_long_read(&n->total_objects);
+			else if (flags & SO_OBJECTS)
+				x = atomic_long_read(&n->total_objects) -
+					count_partial(n, count_free);
 			else
 				x = atomic_long_read(&n->nr_slabs);
 			total += x;

@@ -5139,7 +5158,8 @@ static char *create_unique_id(struct kmem_cache *s)
 
 #ifdef CONFIG_MEMCG_KMEM
 	if (!is_root_cache(s))
-		p += sprintf(p, "-%08d", memcg_cache_id(s->memcg_params->memcg));
+		p += sprintf(p, "-%08d",
+				memcg_cache_id(s->memcg_params->memcg));
 #endif
 
 	BUG_ON(p > name + ID_STR_LENGTH - 1);

Signature · 7 lines

1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

Acked-by on v1 1/2

2013-07-15 14:19 UTC #6 lore raw

Re: [PATCH 1/2] mm/slub.c: beautify code for 80 column limitation and tab alignment.

Quoted 1 line · Chen Gang

Be sure of 80 column limitation for both code and comments.

Acked-by Christoph Lameter <[email protected]>

Chen Gang

author

2013-07-16 00:56 UTC #7 lore raw

Re: [PATCH 1/2] mm/slub.c: beautify code for 80 column limitation and tab alignment.

Quoted 8 lines · Christoph Lameter

Quoted 1 line · Chen Gang
Be sure of 80 column limitation for both code and comments.
Acked-by Christoph Lameter <[email protected]>

Thanks. :-)

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Chen Gang

author

2013-07-15 01:07 UTC #8 lore raw

PATCH 2/2 mm/slub.c: beautify code for removing redundancy 'break' statement.

In reply to Christoph Lameter

Remove redundancy 'break' statement.

Signed-off-by Chen Gang <[email protected]>

---
 mm/slub.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

mm/slub.c +0 -1

index 05ab2d5..db93fa4 100644
@@ -878,7 +878,6 @@ static int on_freelist(struct kmem_cache *s, struct page *page, void *search)
 				object_err(s, page, object,
 					"Freechain corrupt");
 				set_freepointer(s, object, NULL);
-				break;
 			} else {
 				slab_err(s, page, "Freepointer corrupt");
 				page->freelist = NULL;

Signature · 7 lines

1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2013-07-17 07:08 UTC #9 lore raw

Re: [PATCH 2/2] mm/slub.c: beautify code for removing redundancy 'break' statement.

Quoted 3 lines · Chen Gang

Remove redundancy 'break' statement.
Signed-off-by Chen Gang <[email protected]>

Christoph?

Quoted 19 lines

---
 mm/slub.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

mm/slub.c +0 -2

index 05ab2d5..db93fa4 100644
@@ -878,7 +878,6 @@ static int on_freelist(struct kmem_cache *s, struct page *page, void *search)
                                object_err(s, page, object,
                                        "Freechain corrupt");
                                set_freepointer(s, object, NULL);
-                               break;
                        } else {
                                slab_err(s, page, "Freepointer corrupt");
                                page->freelist = NULL;
--

1.7.7.6

Quoted 5 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

Acked-by on v1 2/2

2013-07-17 14:47 UTC #10 lore raw

Re: [PATCH 2/2] mm/slub.c: beautify code for removing redundancy 'break' statement.

In reply to Chen Gang

Quoted 1 line · Chen Gang

Remove redundancy 'break' statement.

Acked-by Christoph Lameter <[email protected]>

Chen Gang

author

2013-07-18 00:03 UTC #11 lore raw

Re: [PATCH 2/2] mm/slub.c: beautify code for removing redundancy 'break' statement.

Quoted 7 lines · Christoph Lameter

Quoted 1 line · Chen Gang
Remove redundancy 'break' statement.
Acked-by Christoph Lameter <[email protected]>

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-07-12 13:53 UTC #12 lore raw

In reply to Chen Gang

Quoted 3 lines · Chen Gang

Since all values which can be assigned to 'slub_debug' are 'unsigned long', recommend also to define 'slub_debug' as 'unsigned long' to match the type precisely

The bit definitions in slab.h as well as slub.c all assume that these are 32 bit entities. See f.e. the defition of the internal slub flags:

/* Internal SLUB flags */
#define __OBJECT_POISON 0x80000000UL /* Poison object */
#define __CMPXCHG_DOUBLE 0x40000000UL /* Use cmpxchg_double */

Chen Gang

author

2013-07-15 00:20 UTC #13 lore raw

Quoted 6 lines · Christoph Lameter

Quoted 3 lines · Chen Gang
Since all values which can be assigned to 'slub_debug' are 'unsigned long', recommend also to define 'slub_debug' as 'unsigned long' to match the type precisely

Quoted 7 lines

The bit definitions in slab.h as well as slub.c all assume that these are 32 bit entities. See f.e. the defition of the internal slub flags:
/* Internal SLUB flags */
#define __OBJECT_POISON 0x80000000UL /* Poison object */
#define __CMPXCHG_DOUBLE 0x40000000UL /* Use cmpxchg_double */

As far as I know, 'UL' means "unsigned long", is it correct ?

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-07-15 14:17 UTC #14 lore raw

Quoted 10 lines · Chen Gang

Quoted 9 lines · Christoph Lameter
Quoted 3 lines · Chen Gang
Since all values which can be assigned to 'slub_debug' are 'unsigned long', recommend also to define 'slub_debug' as 'unsigned long' to match the type precisely
The bit definitions in slab.h as well as slub.c all assume that these are 32 bit entities. See f.e. the defition of the internal slub flags:

Quoted 6 lines

Quoted 4 lines
/* Internal SLUB flags */
#define __OBJECT_POISON 0x80000000UL /* Poison object */
#define __CMPXCHG_DOUBLE 0x40000000UL /* Use cmpxchg_double */
As far as I know, 'UL' means "unsigned long", is it correct ?

This is the way hex constants are generally specified.

Chen Gang

author

2013-07-16 00:54 UTC #15 lore raw

Quoted 12 lines · Christoph Lameter

Quoted 10 lines · Chen Gang
Quoted 10 lines · Christoph Lameter
Quoted 9 lines
Quoted 9 lines · Chen Gang
Quoted 4 lines
Quoted 4 lines
Since all values which can be assigned to 'slub_debug' are 'unsigned long', recommend also to define 'slub_debug' as 'unsigned long' to match the type precisely
The bit definitions in slab.h as well as slub.c all assume that these are 32 bit entities. See f.e. the defition of the internal slub flags:

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
/* Internal SLUB flags */
#define __OBJECT_POISON 0x80000000UL /* Poison object */
#define __CMPXCHG_DOUBLE 0x40000000UL /* Use cmpxchg_double */
As far as I know, 'UL' means "unsigned long", is it correct ?
This is the way hex constants are generally specified.

The C compiler will treat 'UL' as "unsigned long".

If we really use 32-bit as unsigned number, better to use 'U' instead of 'UL' (e.g. 0x80000000U instead of 0x80000000UL).

Since it is unsigned 32-bit number, it is better to use 'unsigned int' instead of 'int', which can avoid related warnings if "EXTRA_CFLAGS=-W".

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-07-17 14:46 UTC #16 lore raw

Quoted 5 lines · Chen Gang

If we really use 32-bit as unsigned number, better to use 'U' instead of 'UL' (e.g. 0x80000000U instead of 0x80000000UL).
Since it is unsigned 32-bit number, it is better to use 'unsigned int' instead of 'int', which can avoid related warnings if "EXTRA_CFLAGS=-W".

Ok could you go through the kernel source and change that?

Chen Gang

author

2013-07-18 00:14 UTC #17 lore raw

Quoted 10 lines · Christoph Lameter

Quoted 5 lines · Chen Gang
If we really use 32-bit as unsigned number, better to use 'U' instead of 'UL' (e.g. 0x80000000U instead of 0x80000000UL).
Since it is unsigned 32-bit number, it is better to use 'unsigned int' instead of 'int', which can avoid related warnings if "EXTRA_CFLAGS=-W".
Ok could you go through the kernel source and change that?

Yeah, thanks, I should do it.

Hmm... for each case of this issue, it need communicate with (review by) various related maintainers.

So, I think one patch for one variable (and related macro contents) is enough.

Is it OK ?

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-07-18 13:42 UTC #18 lore raw

Quoted 14 lines · Chen Gang

Quoted 10 lines · Christoph Lameter
Quoted 5 lines · Chen Gang
If we really use 32-bit as unsigned number, better to use 'U' instead of 'UL' (e.g. 0x80000000U instead of 0x80000000UL).
Since it is unsigned 32-bit number, it is better to use 'unsigned int' instead of 'int', which can avoid related warnings if "EXTRA_CFLAGS=-W".
Ok could you go through the kernel source and change that?
Yeah, thanks, I should do it.

Quoted 7 lines

Hmm... for each case of this issue, it need communicate with (review by) various related maintainers.
So, I think one patch for one variable (and related macro contents) is enough.
Is it OK ?

The fundamental issue is that typically ints are used for flags and I would like to keep it that way. Changing the constants in slab.h and the allocator code to be unsigned int instead of unsigned long wont be that much of a deal.

Will the code then be clean enough for you?

Chen Gang F T

2013-07-19 00:52 UTC #19 lore raw

Quoted 22 lines · Christoph Lameter

Quoted 20 lines · Chen Gang
Quoted 10 lines · Christoph Lameter
Quoted 5 lines · Chen Gang
If we really use 32-bit as unsigned number, better to use 'U' instead of 'UL' (e.g. 0x80000000U instead of 0x80000000UL).
Since it is unsigned 32-bit number, it is better to use 'unsigned int' instead of 'int', which can avoid related warnings if "EXTRA_CFLAGS=-W".
Ok could you go through the kernel source and change that?
Yeah, thanks, I should do it.
Hmm... for each case of this issue, it need communicate with (review by) various related maintainers.
So, I think one patch for one variable (and related macro contents) is enough.

Quoted 7 lines

Quoted 1 line
Is it OK ?
The fundamental issue is that typically ints are used for flags and I would like to keep it that way. Changing the constants in slab.h and the allocator code to be unsigned int instead of unsigned long wont be that much of a deal.

At least, we need use 'unsigned' instead of 'signed'.

e.g.
----------------------------diff begin---------------------------------

mm/slub.c +2 -2

index 2b02d66..7111d7a 100644
@@ -452,9 +452,9 @@ static void get_map(struct kmem_cache *s, struct page *page, unsigned long *map)
  * Debug settings:
  */
 #ifdef CONFIG_SLUB_DEBUG_ON
-static int slub_debug = DEBUG_DEFAULT_FLAGS;
+static unsigned int slub_debug = DEBUG_DEFAULT_FLAGS;
 #else
-static int slub_debug;
+static unsigned int slub_debug;
 #endif
 
 static char *slub_debug_slabs;

----------------------------diff end-----------------------------------

Quoted 2 lines

Will the code then be clean enough for you?

Hmm... Things maybe seem more complex, please see bellow:

For 'SLAB_RED_ZONE' (or the other constants), they also can be assigned to "struct kmem_cache" member variable 'flags'.

But for "struct kmem_cache", it has 2 different definitions, they share with the 'SLAB_RED_ZONE' (or the other constants).

One defines 'flags' as 'unsigned int' in "include/linux/slab_def.h"

16 /*

 17  * struct kmem_cache
 18  *
 19  * manages a cache.
 20  */
 21 
 22 struct kmem_cache {
 23 /* 1) Cache tunables. Protected by cache_chain_mutex */
 24         unsigned int batchcount;
 25         unsigned int limit;
 26         unsigned int shared;
 27 
 28         unsigned int size;
 29         u32 reciprocal_buffer_size;
 30 /* 2) touched by every alloc & free from the backend */
 31 
 32         unsigned int flags;             /* constant flags */
 33         unsigned int num;               /* # of objs per slab */

...

The other defines 'flags' as 'unsigned long' in "include/linux/slub_def.h" (but from its comments, it even says it is for 'Slab' cache management !!)

65 /*
66 * Slab cache management.
67 */
68 struct kmem_cache {

 69         struct kmem_cache_cpu __percpu *cpu_slab;
 70         /* Used for retriving partial slabs etc */
 71         unsigned long flags;
 72         unsigned long min_partial;
 73         int size;               /* The size of an object including meta data */
 74         int object_size;        /* The size of an object without meta data */
 75         int offset;             /* Free pointer offset. */
 76         int cpu_partial;        /* Number of per cpu partial objects to keep around */
 77         struct kmem_cache_order_objects oo;

...

Maybe it is also related with our discussion ('unsigned int' or 'unsigned long') ?

Quoted 6 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-07-19 14:00 UTC #20 lore raw

Quoted 7 lines · Chen Gang F T

Quoted 5 lines
The fundamental issue is that typically ints are used for flags and I would like to keep it that way. Changing the constants in slab.h and the allocator code to be unsigned int instead of unsigned long wont be that much of a deal.
At least, we need use 'unsigned' instead of 'signed'.

Ok.

Quoted 28 lines

Hmm... Things maybe seem more complex, please see bellow:
For 'SLAB_RED_ZONE' (or the other constants), they also can be assigned to "struct kmem_cache" member variable 'flags'.
But for "struct kmem_cache", it has 2 different definitions, they share with the 'SLAB_RED_ZONE' (or the other constants).
One defines 'flags' as 'unsigned int' in "include/linux/slab_def.h"
16 /*
 17  * struct kmem_cache
 18  *
 19  * manages a cache.
 20  */
 21
 22 struct kmem_cache {
 23 /* 1) Cache tunables. Protected by cache_chain_mutex */
 24         unsigned int batchcount;
 25         unsigned int limit;
 26         unsigned int shared;
 27
 28         unsigned int size;
 29         u32 reciprocal_buffer_size;
 30 /* 2) touched by every alloc & free from the backend */
 31
 32         unsigned int flags;             /* constant flags */
 33         unsigned int num;               /* # of objs per slab */

Quoted 4 lines

...
The other defines 'flags' as 'unsigned long' in "include/linux/slub_def.h" (but from its comments, it even says it is for 'Slab' cache management !!)

SLUB is slab allocator so there is nothing wrong with that.

Quoted 1 line

Maybe it is also related with our discussion ('unsigned int' or 'unsigned long') ?

Well we can make this uniformly unsigned int or long I guess. What would be the benefits of one vs the other?

Chen Gang

author

2013-07-22 01:20 UTC #21 lore raw

Quoted 42 lines · Christoph Lameter

Quoted 7 lines · Chen Gang F T
Quoted 5 lines
The fundamental issue is that typically ints are used for flags and I would like to keep it that way. Changing the constants in slab.h and the allocator code to be unsigned int instead of unsigned long wont be that much of a deal.
At least, we need use 'unsigned' instead of 'signed'.
Ok.
Quoted 30 lines
Hmm... Things maybe seem more complex, please see bellow:
For 'SLAB_RED_ZONE' (or the other constants), they also can be assigned to "struct kmem_cache" member variable 'flags'.
But for "struct kmem_cache", it has 2 different definitions, they share with the 'SLAB_RED_ZONE' (or the other constants).
One defines 'flags' as 'unsigned int' in "include/linux/slab_def.h"
16 /*
 17  * struct kmem_cache
 18  *
 19  * manages a cache.
 20  */
 21
 22 struct kmem_cache {
 23 /* 1) Cache tunables. Protected by cache_chain_mutex */
 24         unsigned int batchcount;
 25         unsigned int limit;
 26         unsigned int shared;
 27
 28         unsigned int size;
 29         u32 reciprocal_buffer_size;
 30 /* 2) touched by every alloc & free from the backend */
 31
 32         unsigned int flags;             /* constant flags */
 33         unsigned int num;               /* # of objs per slab */
...

Quoted 5 lines

Quoted 2 lines
The other defines 'flags' as 'unsigned long' in "include/linux/slub_def.h" (but from its comments, it even says it is for 'Slab' cache management !!)
SLUB is slab allocator so there is nothing wrong with that.

OK, thanks.

Quoted 5 lines

Quoted 1 line
Maybe it is also related with our discussion ('unsigned int' or 'unsigned long') ?
Well we can make this uniformly unsigned int or long I guess. What would be the benefits of one vs the other?

Yeah, need let the 2 'flags' with the same type: "make this uniformly unsigned int or long".

Hmm... Flags variable is always the solid length variable, so it is not suitable to use 'unsigned long' which length depends on 32/64 machine automatically.

If the 'unsigned int' is not enough, we need use 'unsigned long long' instead of. Else (it's enough), better to still use 'unsigned int' to save memory usage.

'unsigned long' is useful(necessary) for some variables (e.g. address related variables), but is not suitable for the always solid length variable.

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>