| Message ID | 535a552cd2c05a3ae2cb61da2583646e1c649699.1395832156.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 03/26/2014 04:51 PM, Viresh Kumar wrote: > In switch_hrtimer_base() we have created a local variable basenum which is set > to base->index. This variable is used at only one place. It makes code more > readable if we remove this variable use base->index directly. > No, this doesn't look right. Note that the code can re-execute the assignment to new_base, by jumping to the 'again' label. See below. > --- a/kernel/hrtimer.c > +++ b/kernel/hrtimer.c > @@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, > struct hrtimer_cpu_base *new_cpu_base; > int this_cpu = smp_processor_id(); > int cpu = get_nohz_timer_target(pinned); > - int basenum = base->index; > > again: > new_cpu_base = &per_cpu(hrtimer_bases, cpu); > - new_base = &new_cpu_base->clock_base[basenum]; > + new_base = &new_cpu_base->clock_base[base->index]; > Further down, timer->base can be altered (and set to NULL too). So if we jump back to 'again', we'll end up in trouble. So I think its important to cache the value in basenum and use it. > if (base != new_base) { > /* > Regards, Srivatsa S. Bhat -- 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/
On 26 March 2014 17:13, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote: >> + new_base = &new_cpu_base->clock_base[base->index]; > > Further down, timer->base can be altered (and set to NULL too). > So if we jump back to 'again', we'll end up in trouble. > So I think its important to cache the value in basenum and > use it. base is a parameter to this function and never changes. So base->index is guaranteed to be valid and same during a functions call. -- 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/
On Wed, 26 Mar 2014, Srivatsa S. Bhat wrote: > On 03/26/2014 04:51 PM, Viresh Kumar wrote: > > In switch_hrtimer_base() we have created a local variable basenum which is set > > to base->index. This variable is used at only one place. It makes code more > > readable if we remove this variable use base->index directly. > > > > No, this doesn't look right. Note that the code can re-execute > the assignment to new_base, by jumping to the 'again' label. > See below. > > > --- a/kernel/hrtimer.c > > +++ b/kernel/hrtimer.c > > @@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, > > struct hrtimer_cpu_base *new_cpu_base; > > int this_cpu = smp_processor_id(); > > int cpu = get_nohz_timer_target(pinned); > > - int basenum = base->index; > > > > again: > > new_cpu_base = &per_cpu(hrtimer_bases, cpu); > > - new_base = &new_cpu_base->clock_base[basenum]; > > + new_base = &new_cpu_base->clock_base[base->index]; > > > > Further down, timer->base can be altered (and set to NULL too). > So if we jump back to 'again', we'll end up in trouble. > So I think its important to cache the value in basenum and > use it. That's irrelevant. base is not changing. -- 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/
On 03/26/2014 11:01 PM, Thomas Gleixner wrote: > On Wed, 26 Mar 2014, Srivatsa S. Bhat wrote: >> On 03/26/2014 04:51 PM, Viresh Kumar wrote: >>> In switch_hrtimer_base() we have created a local variable basenum which is set >>> to base->index. This variable is used at only one place. It makes code more >>> readable if we remove this variable use base->index directly. >>> >> >> No, this doesn't look right. Note that the code can re-execute >> the assignment to new_base, by jumping to the 'again' label. >> See below. >> >>> --- a/kernel/hrtimer.c >>> +++ b/kernel/hrtimer.c >>> @@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, >>> struct hrtimer_cpu_base *new_cpu_base; >>> int this_cpu = smp_processor_id(); >>> int cpu = get_nohz_timer_target(pinned); >>> - int basenum = base->index; >>> >>> again: >>> new_cpu_base = &per_cpu(hrtimer_bases, cpu); >>> - new_base = &new_cpu_base->clock_base[basenum]; >>> + new_base = &new_cpu_base->clock_base[base->index]; >>> >> >> Further down, timer->base can be altered (and set to NULL too). >> So if we jump back to 'again', we'll end up in trouble. >> So I think its important to cache the value in basenum and >> use it. > > That's irrelevant. base is not changing. > Sorry, I missed that :-( Regards, Srivatsa S. Bhat -- 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/
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 95243f2..b0bcc10 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, struct hrtimer_cpu_base *new_cpu_base; int this_cpu = smp_processor_id(); int cpu = get_nohz_timer_target(pinned); - int basenum = base->index; again: new_cpu_base = &per_cpu(hrtimer_bases, cpu); - new_base = &new_cpu_base->clock_base[basenum]; + new_base = &new_cpu_base->clock_base[base->index]; if (base != new_base) { /*
In switch_hrtimer_base() we have created a local variable basenum which is set to base->index. This variable is used at only one place. It makes code more readable if we remove this variable use base->index directly. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- kernel/hrtimer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)