[PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[Andrey Ryabinin]

kmem_cache_zalloc_node() allocates zeroed memory for a particular
cache from a specified memory node. To be used for struct irq_desc.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 include/linux/slab.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index c265bec..b3248fa 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -574,6 +574,12 @@ static inline void *kmem_cache_zalloc(struct kmem_cache *k, gfp_t flags)
 	return kmem_cache_alloc(k, flags | __GFP_ZERO);
 }
 
+static inline void *kmem_cache_zalloc_node(struct kmem_cache *k, gfp_t flags,
+					int node)
+{
+	return kmem_cache_alloc_node(k, flags | __GFP_ZERO, node);
+}
+
 /**
  * kzalloc - allocate memory. The memory is set to zero.
  * @size: how many bytes of memory are required.
-- 
2.1.3

[PATCH 2/3] kernel: irq: use a kmem_cache for allocating struct irq_desc

[Andrey Ryabinin]

After enabling alignment checks in UBSan I've noticed a lot of
reports like this:

    UBSan: Undefined behaviour in ../kernel/irq/chip.c:195:14
    member access within misaligned address ffff88003e80d6f8
    for type 'struct irq_desc' which requires 16 byte alignment

struct irq_desc declared with ____cacheline_internodealigned_in_smp
attribute. However in some cases it allocated dynamically via kmalloc().
In general case kmalloc() guaranties only sizeof(void *) alignment.
We should use a separate slab cache to make struct irq_desc
properly aligned on SMP configuration.

This also could slightly reduce memory usage on some configurations.
E.g. in my setup sizeof(struct irq_desc) == 320. Which means that
kmalloc-512 will be used for allocating irg_desc via kmalloc().
In that case using separate slab cache will save us 192 bytes per
each irq_desc.

Note: UBSan reports says that 'struct irq_desc' requires 16 byte alignment.
It's wrong, in my setup it should be 64 bytes. This looks like a gcc bug,
but it doesn't change the fact that irq_desc is misaligned.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 kernel/irq/irqdesc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index a1782f8..f22cb87 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -23,6 +23,8 @@
  */
 static struct lock_class_key irq_desc_lock_class;
 
+static struct kmem_cache *irq_desc_cachep;
+
 #if defined(CONFIG_SMP)
 static void __init init_irq_default_affinity(void)
 {
@@ -137,9 +139,10 @@ static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
 	struct irq_desc *desc;
 	gfp_t gfp = GFP_KERNEL;
 
-	desc = kzalloc_node(sizeof(*desc), gfp, node);
+	desc = kmem_cache_zalloc_node(irq_desc_cachep, gfp, node);
 	if (!desc)
 		return NULL;
+
 	/* allocate based on nr_cpu_ids */
 	desc->kstat_irqs = alloc_percpu(unsigned int);
 	if (!desc->kstat_irqs)
@@ -158,7 +161,7 @@ static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
 err_kstat:
 	free_percpu(desc->kstat_irqs);
 err_desc:
-	kfree(desc);
+	kmem_cache_free(irq_desc_cachep, desc);
 	return NULL;
 }
 
@@ -174,7 +177,7 @@ static void free_desc(unsigned int irq)
 
 	free_masks(desc);
 	free_percpu(desc->kstat_irqs);
-	kfree(desc);
+	kmem_cache_free(irq_desc_cachep, desc);
 }
 
 static int alloc_descs(unsigned int start, unsigned int cnt, int node,
@@ -218,6 +221,8 @@ int __init early_irq_init(void)
 
 	init_irq_default_affinity();
 
+	irq_desc_cachep = KMEM_CACHE(irq_desc, SLAB_PANIC);
+
 	/* Let arch update nr_irqs and return the nr of preallocated irqs */
 	initcnt = arch_probe_nr_irqs();
 	printk(KERN_INFO "NR_IRQS:%d nr_irqs:%d %d\n", NR_IRQS, nr_irqs, initcnt);
-- 
2.1.3

Re: [PATCH 2/3] kernel: irq: use a kmem_cache for allocating struct irq_desc

[David Rientjes]

On Mon, 10 Nov 2014, Andrey Ryabinin wrote:

> After enabling alignment checks in UBSan I've noticed a lot of
> reports like this:
> 
>     UBSan: Undefined behaviour in ../kernel/irq/chip.c:195:14
>     member access within misaligned address ffff88003e80d6f8
>     for type 'struct irq_desc' which requires 16 byte alignment
> 
> struct irq_desc declared with ____cacheline_internodealigned_in_smp
> attribute. However in some cases it allocated dynamically via kmalloc().
> In general case kmalloc() guaranties only sizeof(void *) alignment.
> We should use a separate slab cache to make struct irq_desc
> properly aligned on SMP configuration.
> 
> This also could slightly reduce memory usage on some configurations.
> E.g. in my setup sizeof(struct irq_desc) == 320. Which means that
> kmalloc-512 will be used for allocating irg_desc via kmalloc().
> In that case using separate slab cache will save us 192 bytes per
> each irq_desc.
> 
> Note: UBSan reports says that 'struct irq_desc' requires 16 byte alignment.
> It's wrong, in my setup it should be 64 bytes. This looks like a gcc bug,
> but it doesn't change the fact that irq_desc is misaligned.
> 
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>

I think this is just fine, I would just prefer that you do the memset() 
explicitly rather than introduce the new slab function for such a 
specialized purpose (unless there's other examples in the kernel where 
this would be useful).

Re: [PATCH 2/3] kernel: irq: use a kmem_cache for allocating struct irq_desc

[Andrey Ryabinin]

On 11/20/2014 02:52 AM, David Rientjes wrote:
> On Mon, 10 Nov 2014, Andrey Ryabinin wrote:
> 
>> After enabling alignment checks in UBSan I've noticed a lot of
>> reports like this:
>>
>>     UBSan: Undefined behaviour in ../kernel/irq/chip.c:195:14
>>     member access within misaligned address ffff88003e80d6f8
>>     for type 'struct irq_desc' which requires 16 byte alignment
>>
>> struct irq_desc declared with ____cacheline_internodealigned_in_smp
>> attribute. However in some cases it allocated dynamically via kmalloc().
>> In general case kmalloc() guaranties only sizeof(void *) alignment.
>> We should use a separate slab cache to make struct irq_desc
>> properly aligned on SMP configuration.
>>
>> This also could slightly reduce memory usage on some configurations.
>> E.g. in my setup sizeof(struct irq_desc) == 320. Which means that
>> kmalloc-512 will be used for allocating irg_desc via kmalloc().
>> In that case using separate slab cache will save us 192 bytes per
>> each irq_desc.
>>
>> Note: UBSan reports says that 'struct irq_desc' requires 16 byte alignment.
>> It's wrong, in my setup it should be 64 bytes. This looks like a gcc bug,
>> but it doesn't change the fact that irq_desc is misaligned.
>>
>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> 
> I think this is just fine, I would just prefer that you do the memset() 

I'd rather do kmem_cache_alloc_node(irq_desc_cachep, gfp | __GFP_ZERO, node)
instead of memset.

> explicitly rather than introduce the new slab function for such a 
> specialized purpose (unless there's other examples in the kernel where 
> this would be useful).
> 

I've counted 7 places where kmem_cache_alloc_node(..., gfp | __GFP_ZERO, ...); called.

[PATCH 3/3] kernel: irq: use kmem_cache for allocating struct irqaction

[Andrey Ryabinin]

After enabling alignment checks in UBSan I've noticed several
reports like this:

   UBSan: Undefined behaviour in kernel/irq/manage.c:1315:13
   member access within misaligned address ffff88007c274558
   for type 'struct irqaction' which requires 16 byte alignment

struct irqaction declared with ____cacheline_internodealigned_in_smp
attribute. However in some cases it allocated dynamically via kmalloc().
In general case kmalloc() guaranties only sizeof(void *) alignment.
We should use a separate slab cache to make struct irqaction
properly aligned on SMP configuration.

Note: UBSan reports says that 'struct irqaction' requires 16 byte alignment.
It's wrong, in my setup it should be 64 bytes. This looks like a gcc bug,
but it doesn't change the fact that irqaction is misaligned.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 kernel/irq/internals.h |  2 ++
 kernel/irq/irqdesc.c   |  1 +
 kernel/irq/manage.c    | 14 ++++++++------
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 4332d76..95b61c5 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -7,6 +7,7 @@
  */
 #include <linux/irqdesc.h>
 #include <linux/kernel_stat.h>
+#include <linux/slab.h>
 
 #ifdef CONFIG_SPARSE_IRQ
 # define IRQ_BITMAP_BITS	(NR_IRQS + 8196)
@@ -17,6 +18,7 @@
 #define istate core_internal_state__do_not_mess_with_it
 
 extern bool noirqdebug;
+extern struct kmem_cache *irqaction_cachep;
 
 /*
  * Bits used by threaded handlers:
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index f22cb87..52c3e4f 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -222,6 +222,7 @@ int __init early_irq_init(void)
 	init_irq_default_affinity();
 
 	irq_desc_cachep = KMEM_CACHE(irq_desc, SLAB_PANIC);
+	irqaction_cachep = KMEM_CACHE(irqaction, SLAB_PANIC);
 
 	/* Let arch update nr_irqs and return the nr of preallocated irqs */
 	initcnt = arch_probe_nr_irqs();
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 0a9104b..7c69597 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -21,6 +21,8 @@
 
 #include "internals.h"
 
+struct kmem_cache *irqaction_cachep;
+
 #ifdef CONFIG_IRQ_FORCED_THREADING
 __read_mostly bool force_irqthreads;
 
@@ -1409,7 +1411,7 @@ void free_irq(unsigned int irq, void *dev_id)
 #endif
 
 	chip_bus_lock(desc);
-	kfree(__free_irq(irq, dev_id));
+	kmem_cache_free(irqaction_cachep, __free_irq(irq, dev_id));
 	chip_bus_sync_unlock(desc);
 }
 EXPORT_SYMBOL(free_irq);
@@ -1487,7 +1489,7 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 		handler = irq_default_primary_handler;
 	}
 
-	action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
+	action = kmem_cache_zalloc(irqaction_cachep, GFP_KERNEL);
 	if (!action)
 		return -ENOMEM;
 
@@ -1502,7 +1504,7 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	chip_bus_sync_unlock(desc);
 
 	if (retval)
-		kfree(action);
+		kmem_cache_free(irqaction_cachep, action);
 
 #ifdef CONFIG_DEBUG_SHIRQ_FIXME
 	if (!retval && (irqflags & IRQF_SHARED)) {
@@ -1683,7 +1685,7 @@ void free_percpu_irq(unsigned int irq, void __percpu *dev_id)
 		return;
 
 	chip_bus_lock(desc);
-	kfree(__free_percpu_irq(irq, dev_id));
+	kmem_cache_free(irqaction_cachep, __free_percpu_irq(irq, dev_id));
 	chip_bus_sync_unlock(desc);
 }
 
@@ -1738,7 +1740,7 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 	    !irq_settings_is_per_cpu_devid(desc))
 		return -EINVAL;
 
-	action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
+	action = kmem_cache_zalloc(irqaction_cachep, GFP_KERNEL);
 	if (!action)
 		return -ENOMEM;
 
@@ -1752,7 +1754,7 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 	chip_bus_sync_unlock(desc);
 
 	if (retval)
-		kfree(action);
+		kmem_cache_free(irqaction_cachep, action);
 
 	return retval;
 }
-- 
2.1.3

Re: [PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[David Rientjes]

On Mon, 10 Nov 2014, Andrey Ryabinin wrote:

> kmem_cache_zalloc_node() allocates zeroed memory for a particular
> cache from a specified memory node. To be used for struct irq_desc.
> 

Is there a reason to add this for such a specialized purpose to the slab 
allocator?  I think it can just be handled for struct irq_desc explicitly.

Re: [PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[Andrey Ryabinin]

On 11/20/2014 02:46 AM, David Rientjes wrote:
> On Mon, 10 Nov 2014, Andrey Ryabinin wrote:
> 
>> kmem_cache_zalloc_node() allocates zeroed memory for a particular
>> cache from a specified memory node. To be used for struct irq_desc.
>>
> 
> Is there a reason to add this for such a specialized purpose to the slab 
> allocator?  I think it can just be handled for struct irq_desc explicitly.
> 

It could be used not only for irq_desc. Grepping sources gave me 7 possible users.

We already have zeroing variants of kmalloc/kmalloc_node/kmem_cache_alloc,
so why kmem_cache_alloc_node is special?

Re: [PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[Christoph Lameter]

On Thu, 20 Nov 2014, Andrey Ryabinin wrote:

> It could be used not only for irq_desc. Grepping sources gave me 7 possible users.
>
> We already have zeroing variants of kmalloc/kmalloc_node/kmem_cache_alloc,
> so why kmem_cache_alloc_node is special?

Why do we need this at all? You can always add the __GFP_ZERO flag and any
alloc function will then zero the memory for you.

Re: [PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[David Rientjes]

On Thu, 20 Nov 2014, Andrey Ryabinin wrote:

> > Is there a reason to add this for such a specialized purpose to the slab 
> > allocator?  I think it can just be handled for struct irq_desc explicitly.
> > 
> 
> It could be used not only for irq_desc. Grepping sources gave me 7 possible users.
> 

It would be best to follow in the example of those users and just use 
__GFP_ZERO.

Re: [PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[Andrey Ryabinin]

On 11/21/2014 01:31 AM, David Rientjes wrote:
> On Thu, 20 Nov 2014, Andrey Ryabinin wrote:
> 
>>> Is there a reason to add this for such a specialized purpose to the slab 
>>> allocator?  I think it can just be handled for struct irq_desc explicitly.
>>>
>>
>> It could be used not only for irq_desc. Grepping sources gave me 7 possible users.
>>
> 
> It would be best to follow in the example of those users and just use 
> __GFP_ZERO.
> 

Fair enough.

Re: [PATCH 1/3] mm: sl[aou]b: introduce kmem_cache_zalloc_node()

[David Rientjes]

On Fri, 21 Nov 2014, Andrey Ryabinin wrote:

> On 11/21/2014 01:31 AM, David Rientjes wrote:
> > On Thu, 20 Nov 2014, Andrey Ryabinin wrote:
> > 
> >>> Is there a reason to add this for such a specialized purpose to the slab 
> >>> allocator?  I think it can just be handled for struct irq_desc explicitly.
> >>>
> >>
> >> It could be used not only for irq_desc. Grepping sources gave me 7 possible users.
> >>
> > 
> > It would be best to follow in the example of those users and just use 
> > __GFP_ZERO.
> > 
> 
> Fair enough.
> 

Thanks, and feel free to add my

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

on the other two patches once they are refreshed.