From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
linaro-kernel@lists.linaro.org, linux-kernel@vger.kernel.org,
fweisbec@gmail.com, linaro-networking@linaro.org
Subject: Re: [PATCH 04/14] hrtimer: use base->index instead of basenum in switch_hrtimer_base()
Date: Wed, 26 Mar 2014 23:27:43 +0530 [thread overview]
Message-ID: <53331517.1000402@linux.vnet.ibm.com> (raw)
In-Reply-To: <alpine.DEB.2.02.1403261830400.21095@ionos.tec.linutronix.de>
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
next prev parent reply other threads:[~2014-03-26 17:58 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-26 11:21 [PATCH 00/14] timers/hrtimers: Minor cleanups Viresh Kumar
2014-03-26 11:21 ` [PATCH 01/14] hrtimer: replace 'tab' with 'space' after 'comma' Viresh Kumar
2014-03-26 11:21 ` [PATCH 02/14] hrtimer: Coalesce format fragments in printk() Viresh Kumar
2014-03-26 11:21 ` [PATCH 03/14] hrtimer: call hrtimer_set_expires_range() from hrtimer_set_expires_range_ns() Viresh Kumar
2014-03-26 11:21 ` [PATCH 04/14] hrtimer: use base->index instead of basenum in switch_hrtimer_base() Viresh Kumar
2014-03-26 11:43 ` Srivatsa S. Bhat
2014-03-26 14:08 ` [LNG] " Viresh Kumar
2014-03-26 17:31 ` Thomas Gleixner
2014-03-26 17:57 ` Srivatsa S. Bhat [this message]
2014-03-26 11:21 ` [PATCH 05/14] hrtimer: no need to rewrite '1' to hrtimer_hres_enabled Viresh Kumar
2014-03-26 11:21 ` [PATCH 06/14] hrtimer: don't rewrite same value to expires_next in hrtimer_force_reprogram() Viresh Kumar
2014-03-26 11:21 ` [PATCH 07/14] hrtimer: use base->hres_active directly instead of hrtimer_hres_active() Viresh Kumar
2014-03-26 11:21 ` [PATCH 08/14] hrtimer: remove dummy definition of hrtimer_force_reprogram() Viresh Kumar
2014-03-26 11:21 ` [PATCH 09/14] hrtimer: don't check state of base->hres_active in hrtimer_switch_to_hres() Viresh Kumar
2014-03-26 11:21 ` [PATCH 10/14] hrtimer: remove clock_was_set_delayed() from hrtimer.h Viresh Kumar
2014-03-26 11:21 ` [PATCH 11/14] hrtimer: remove active_bases field from struct hrtimer_cpu_base Viresh Kumar
2014-03-26 17:28 ` Thomas Gleixner
2014-03-27 4:30 ` Viresh Kumar
2014-03-26 11:21 ` [PATCH 12/14] hrtimer: don't emulate notifier call to initialize timer base Viresh Kumar
2014-03-26 12:40 ` Srivatsa S. Bhat
2014-03-26 14:17 ` [LNG] " Viresh Kumar
2014-03-26 11:21 ` [PATCH 13/14] timer: simplify CPU_UP_PREPARE notifier code path Viresh Kumar
2014-03-26 12:47 ` Srivatsa S. Bhat
2014-03-26 11:21 ` [PATCH 14/14] timer: don't emulate notifier call to initialize timer base Viresh Kumar
2014-03-26 12:43 ` Srivatsa S. Bhat
2014-03-27 5:23 ` [PATCH 1/2] hrtimer: don't add clock-base to active_bases if already present Viresh Kumar
2014-03-27 5:23 ` [PATCH V2 2/2] hrtimer: use __ffs() to iterate over valid bits from active_bases Viresh Kumar
2014-03-27 5:40 ` Thomas Gleixner
2014-03-27 5:46 ` Viresh Kumar
2014-03-27 5:49 ` [PATCH V3] " Viresh Kumar
2014-03-27 5:54 ` [PATCH V3 Resend] hrtimer: use ffs() " Viresh Kumar
2014-03-27 5:39 ` [PATCH 1/2] hrtimer: don't add clock-base to active_bases if already present Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53331517.1000402@linux.vnet.ibm.com \
--to=srivatsa.bhat@linux.vnet.ibm.com \
--cc=fweisbec@gmail.com \
--cc=linaro-kernel@lists.linaro.org \
--cc=linaro-networking@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.