* [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
@ 2012-06-20 9:00 ` Dong Aisheng
0 siblings, 0 replies; 12+ messages in thread
From: Dong Aisheng @ 2012-06-20 9:00 UTC (permalink / raw)
To: linux-arm-kernel
From: Dong Aisheng <dong.aisheng@linaro.org>
There're two copies of irq_desc initialization code, reform them into
an irq_desc_initialize function to call.
Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
kernel/irq/irqdesc.c | 51 +++++++++++++++++++++++++++----------------------
1 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 192a302..e29db67 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -131,34 +131,43 @@ static void free_masks(struct irq_desc *desc)
static inline void free_masks(struct irq_desc *desc) { }
#endif
-static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
+static inline int irq_desc_initialize(struct irq_desc *desc,
+ int irq, int node, struct module *owner)
{
- struct irq_desc *desc;
- gfp_t gfp = GFP_KERNEL;
-
- desc = kzalloc_node(sizeof(*desc), gfp, node);
- if (!desc)
- return NULL;
/* allocate based on nr_cpu_ids */
desc->kstat_irqs = alloc_percpu(unsigned int);
if (!desc->kstat_irqs)
- goto err_desc;
+ return -ENOMEM;
- if (alloc_masks(desc, gfp, node))
- goto err_kstat;
+ if (alloc_masks(desc, GFP_KERNEL, node)) {
+ kfree(desc->kstat_irqs);
+ return -ENOMEM;
+ }
raw_spin_lock_init(&desc->lock);
lockdep_set_class(&desc->lock, &irq_desc_lock_class);
desc_set_defaults(irq, desc, node, owner);
- return desc;
+ return 0;
+}
-err_kstat:
- free_percpu(desc->kstat_irqs);
-err_desc:
- kfree(desc);
- return NULL;
+static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
+{
+ struct irq_desc *desc;
+ int ret;
+
+ desc = kzalloc_node(sizeof(*desc), GFP_KERNEL, node);
+ if (!desc)
+ return NULL;
+
+ ret = irq_desc_initialize(desc, irq, node, owner);
+ if (ret) {
+ kfree(desc);
+ return NULL;
+ }
+
+ return desc;
}
static void free_desc(unsigned int irq)
@@ -260,13 +269,9 @@ int __init early_irq_init(void)
desc = irq_desc;
count = ARRAY_SIZE(irq_desc);
- for (i = 0; i < count; i++) {
- desc[i].kstat_irqs = alloc_percpu(unsigned int);
- alloc_masks(&desc[i], GFP_KERNEL, node);
- raw_spin_lock_init(&desc[i].lock);
- lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
- desc_set_defaults(i, &desc[i], node, NULL);
- }
+ for (i = 0; i < count; i++)
+ irq_desc_initialize(desc, irq, node, NULL);
+
return arch_early_irq_init();
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
2012-06-20 9:00 ` Dong Aisheng
@ 2012-07-06 9:18 ` Dong Aisheng
-1 siblings, 0 replies; 12+ messages in thread
From: Dong Aisheng @ 2012-07-06 9:18 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, grant.likely, tglx, benh
On Wed, Jun 20, 2012 at 05:00:31PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> There're two copies of irq_desc initialization code, reform them into
> an irq_desc_initialize function to call.
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> kernel/irq/irqdesc.c | 51 +++++++++++++++++++++++++++----------------------
> 1 files changed, 28 insertions(+), 23 deletions(-)
>
Ping...
Regards
Dong Aisheng
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 192a302..e29db67 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -131,34 +131,43 @@ static void free_masks(struct irq_desc *desc)
> static inline void free_masks(struct irq_desc *desc) { }
> #endif
>
> -static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
> +static inline int irq_desc_initialize(struct irq_desc *desc,
> + int irq, int node, struct module *owner)
> {
> - struct irq_desc *desc;
> - gfp_t gfp = GFP_KERNEL;
> -
> - desc = kzalloc_node(sizeof(*desc), gfp, node);
> - if (!desc)
> - return NULL;
> /* allocate based on nr_cpu_ids */
> desc->kstat_irqs = alloc_percpu(unsigned int);
> if (!desc->kstat_irqs)
> - goto err_desc;
> + return -ENOMEM;
>
> - if (alloc_masks(desc, gfp, node))
> - goto err_kstat;
> + if (alloc_masks(desc, GFP_KERNEL, node)) {
> + kfree(desc->kstat_irqs);
> + return -ENOMEM;
> + }
>
> raw_spin_lock_init(&desc->lock);
> lockdep_set_class(&desc->lock, &irq_desc_lock_class);
>
> desc_set_defaults(irq, desc, node, owner);
>
> - return desc;
> + return 0;
> +}
>
> -err_kstat:
> - free_percpu(desc->kstat_irqs);
> -err_desc:
> - kfree(desc);
> - return NULL;
> +static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
> +{
> + struct irq_desc *desc;
> + int ret;
> +
> + desc = kzalloc_node(sizeof(*desc), GFP_KERNEL, node);
> + if (!desc)
> + return NULL;
> +
> + ret = irq_desc_initialize(desc, irq, node, owner);
> + if (ret) {
> + kfree(desc);
> + return NULL;
> + }
> +
> + return desc;
> }
>
> static void free_desc(unsigned int irq)
> @@ -260,13 +269,9 @@ int __init early_irq_init(void)
> desc = irq_desc;
> count = ARRAY_SIZE(irq_desc);
>
> - for (i = 0; i < count; i++) {
> - desc[i].kstat_irqs = alloc_percpu(unsigned int);
> - alloc_masks(&desc[i], GFP_KERNEL, node);
> - raw_spin_lock_init(&desc[i].lock);
> - lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
> - desc_set_defaults(i, &desc[i], node, NULL);
> - }
> + for (i = 0; i < count; i++)
> + irq_desc_initialize(desc, irq, node, NULL);
> +
> return arch_early_irq_init();
> }
>
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 12+ messages in thread* [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
@ 2012-07-06 9:18 ` Dong Aisheng
0 siblings, 0 replies; 12+ messages in thread
From: Dong Aisheng @ 2012-07-06 9:18 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 20, 2012 at 05:00:31PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> There're two copies of irq_desc initialization code, reform them into
> an irq_desc_initialize function to call.
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> kernel/irq/irqdesc.c | 51 +++++++++++++++++++++++++++----------------------
> 1 files changed, 28 insertions(+), 23 deletions(-)
>
Ping...
Regards
Dong Aisheng
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 192a302..e29db67 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -131,34 +131,43 @@ static void free_masks(struct irq_desc *desc)
> static inline void free_masks(struct irq_desc *desc) { }
> #endif
>
> -static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
> +static inline int irq_desc_initialize(struct irq_desc *desc,
> + int irq, int node, struct module *owner)
> {
> - struct irq_desc *desc;
> - gfp_t gfp = GFP_KERNEL;
> -
> - desc = kzalloc_node(sizeof(*desc), gfp, node);
> - if (!desc)
> - return NULL;
> /* allocate based on nr_cpu_ids */
> desc->kstat_irqs = alloc_percpu(unsigned int);
> if (!desc->kstat_irqs)
> - goto err_desc;
> + return -ENOMEM;
>
> - if (alloc_masks(desc, gfp, node))
> - goto err_kstat;
> + if (alloc_masks(desc, GFP_KERNEL, node)) {
> + kfree(desc->kstat_irqs);
> + return -ENOMEM;
> + }
>
> raw_spin_lock_init(&desc->lock);
> lockdep_set_class(&desc->lock, &irq_desc_lock_class);
>
> desc_set_defaults(irq, desc, node, owner);
>
> - return desc;
> + return 0;
> +}
>
> -err_kstat:
> - free_percpu(desc->kstat_irqs);
> -err_desc:
> - kfree(desc);
> - return NULL;
> +static struct irq_desc *alloc_desc(int irq, int node, struct module *owner)
> +{
> + struct irq_desc *desc;
> + int ret;
> +
> + desc = kzalloc_node(sizeof(*desc), GFP_KERNEL, node);
> + if (!desc)
> + return NULL;
> +
> + ret = irq_desc_initialize(desc, irq, node, owner);
> + if (ret) {
> + kfree(desc);
> + return NULL;
> + }
> +
> + return desc;
> }
>
> static void free_desc(unsigned int irq)
> @@ -260,13 +269,9 @@ int __init early_irq_init(void)
> desc = irq_desc;
> count = ARRAY_SIZE(irq_desc);
>
> - for (i = 0; i < count; i++) {
> - desc[i].kstat_irqs = alloc_percpu(unsigned int);
> - alloc_masks(&desc[i], GFP_KERNEL, node);
> - raw_spin_lock_init(&desc[i].lock);
> - lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
> - desc_set_defaults(i, &desc[i], node, NULL);
> - }
> + for (i = 0; i < count; i++)
> + irq_desc_initialize(desc, irq, node, NULL);
> +
> return arch_early_irq_init();
> }
>
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
2012-06-20 9:00 ` Dong Aisheng
@ 2012-07-11 22:19 ` Thomas Gleixner
-1 siblings, 0 replies; 12+ messages in thread
From: Thomas Gleixner @ 2012-07-11 22:19 UTC (permalink / raw)
To: Dong Aisheng; +Cc: linux-kernel, linux-arm-kernel, grant.likely, benh
On Wed, 20 Jun 2012, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> There're two copies of irq_desc initialization code, reform them into
> an irq_desc_initialize function to call.
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> kernel/irq/irqdesc.c | 51 +++++++++++++++++++++++++++----------------------
> 1 files changed, 28 insertions(+), 23 deletions(-)
We add more code by removing redundant copies?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
@ 2012-07-11 22:19 ` Thomas Gleixner
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Gleixner @ 2012-07-11 22:19 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 20 Jun 2012, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> There're two copies of irq_desc initialization code, reform them into
> an irq_desc_initialize function to call.
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> kernel/irq/irqdesc.c | 51 +++++++++++++++++++++++++++----------------------
> 1 files changed, 28 insertions(+), 23 deletions(-)
We add more code by removing redundant copies?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
2012-07-11 22:19 ` Thomas Gleixner
@ 2012-07-12 3:26 ` Dong Aisheng
-1 siblings, 0 replies; 12+ messages in thread
From: Dong Aisheng @ 2012-07-12 3:26 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: linux-kernel, linux-arm-kernel, grant.likely, benh
Hi Thomas,
Thanks for the review firstly.
On Thu, Jul 12, 2012 at 06:19:18AM +0800, Thomas Gleixner wrote:
> On Wed, 20 Jun 2012, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> >
> > There're two copies of irq_desc initialization code, reform them into
> > an irq_desc_initialize function to call.
> >
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> > kernel/irq/irqdesc.c | 51 +++++++++++++++++++++++++++----------------------
> > 1 files changed, 28 insertions(+), 23 deletions(-)
>
> We add more code by removing redundant copies?
>
I also had this strange question.
I looked the code a bit more, i guess the main problem is that the redundant copies
is not too big, so we can not see great savings.
Compare to the init code of irq_desc in original alloc_desc function,
the new irq_desc_initialize function saves 4 lines.
However, the new function also add 4 lines for defining extra function name,
parameter and etc.
And alloc_desc still needs to call irq_desc_initialize and checking return value
which needs extra 6 lines.
The main saving is another copy of irq_desc initialization in early_irq_init,
but this copy does not check any return values which cause we did not save too much,
only about 4 lines.
Plus extra blan lines added, so totally it does not save more than new added.
However, i was thinking it could make code looks a bit better.
So i sent out this RFC patch.
Do you think if it's reasonable?
BTW, there's an issue in my patch, should change like:
if (alloc_masks(desc, GFP_KERNEL, node)) {
- kfree(desc->kstat_irqs);
+ free_percpu(desc->kstat_irqs);
return -ENOMEM;
}
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 12+ messages in thread* [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
@ 2012-07-12 3:26 ` Dong Aisheng
0 siblings, 0 replies; 12+ messages in thread
From: Dong Aisheng @ 2012-07-12 3:26 UTC (permalink / raw)
To: linux-arm-kernel
Hi Thomas,
Thanks for the review firstly.
On Thu, Jul 12, 2012 at 06:19:18AM +0800, Thomas Gleixner wrote:
> On Wed, 20 Jun 2012, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> >
> > There're two copies of irq_desc initialization code, reform them into
> > an irq_desc_initialize function to call.
> >
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> > kernel/irq/irqdesc.c | 51 +++++++++++++++++++++++++++----------------------
> > 1 files changed, 28 insertions(+), 23 deletions(-)
>
> We add more code by removing redundant copies?
>
I also had this strange question.
I looked the code a bit more, i guess the main problem is that the redundant copies
is not too big, so we can not see great savings.
Compare to the init code of irq_desc in original alloc_desc function,
the new irq_desc_initialize function saves 4 lines.
However, the new function also add 4 lines for defining extra function name,
parameter and etc.
And alloc_desc still needs to call irq_desc_initialize and checking return value
which needs extra 6 lines.
The main saving is another copy of irq_desc initialization in early_irq_init,
but this copy does not check any return values which cause we did not save too much,
only about 4 lines.
Plus extra blan lines added, so totally it does not save more than new added.
However, i was thinking it could make code looks a bit better.
So i sent out this RFC patch.
Do you think if it's reasonable?
BTW, there's an issue in my patch, should change like:
if (alloc_masks(desc, GFP_KERNEL, node)) {
- kfree(desc->kstat_irqs);
+ free_percpu(desc->kstat_irqs);
return -ENOMEM;
}
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 12+ messages in thread