lkml.org 
[lkml]   [2012]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 2/2] irq: add irq_desc_initialize to remove some duplicated lines
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



\
 
 \ /
  Last update: 2012-07-12 06:21    [W:0.360 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site