All of lore.kernel.org
 help / color / mirror / Atom feed
From: Saravana Kannan <skannan@codeaurora.org>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: "Shilimkar, Santosh" <santosh.shilimkar@ti.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	linux-arm-msm@vger.kernel.org
Subject: Re: udelay() broken for SMP cores?
Date: Wed, 21 Apr 2010 16:47:13 -0700	[thread overview]
Message-ID: <4BCF8E81.4080906@codeaurora.org> (raw)
In-Reply-To: <20100421193305.GB26616@n2100.arm.linux.org.uk>

Russell King - ARM Linux wrote:
> On Wed, Apr 21, 2010 at 03:31:03AM -0700, skannan@codeaurora.org wrote:
>>> Well, the assumption is that the CPUs will be running at their fastest
>>> speed at boot time, and therefore loops_per_jiffy will be calibrated
>>> such that we guarantee _at least_ the asked-for delay - which is the
>>> only guarantee udelay has.
>> Even if the boot assumption is true, cpufreq actively changes the
>> loops_per_jiffy value when it changes freq. So, this could still mess up
>> the _at least_ guarantee.
> 
> Actually, it doesn't on SMP - if you build the kernel with SMP enabled,
> the code which fiddles with loops_per_jiffy is disabled.  See the
> #ifndef wrapping around adjust_jiffies() in drivers/cpufreq/cpufreq.c.

My comment above was for the non-SMP case (it was a reply to your 
comment about non-SMP case). In non-SMP case, cpufreq changes LPJ and 
the freq switch can happen while udelay is looping. That would mess up 
the minimum delay guarantee of udelay.

I was aware that cpufreq doesn't change LPJ for SMP. But I think they do 
that because they don't know where the arch specific per-CPU 
loops_per_jiffy is located. They expect the cpufreq driver to do the lpj 
scaling. So, per-CPU lpj is still going to change. At least, that's what 
I took out of the following comment:

/*
  * This function alters the system "loops_per_jiffy" for the clock
  * speed change.  Note that loops_per_jiffy cannot be updated on SMP
  * systems as each CPU might be scaled differently. So, use the arch
  * per-CPU loops_per_jiffy value wherever possible.
  */


> So, on SMP with cpufreq, the global loops_per_jiffy is a fixed value.
> Provided it was calibrated with the CPU running at max clock rate,
> the guarantee is satisfied for all CPUs in the system.

As mentioned earlier, I think the cpufreq driver for that specific arch 
is supposed to handle the LPJ changes. But let's assume that's not true. 
So, wouldn't this still be a problem? You could be doing udelay as if 
you are running at 1 GHz but you are actually running at 100 MHz. I 
would think that would be bad for performance and power (wasting cycles 
without going into WFI, etc).

Thanks,
Saravana

WARNING: multiple messages have this Message-ID (diff)
From: skannan@codeaurora.org (Saravana Kannan)
To: linux-arm-kernel@lists.infradead.org
Subject: udelay() broken for SMP cores?
Date: Wed, 21 Apr 2010 16:47:13 -0700	[thread overview]
Message-ID: <4BCF8E81.4080906@codeaurora.org> (raw)
In-Reply-To: <20100421193305.GB26616@n2100.arm.linux.org.uk>

Russell King - ARM Linux wrote:
> On Wed, Apr 21, 2010 at 03:31:03AM -0700, skannan at codeaurora.org wrote:
>>> Well, the assumption is that the CPUs will be running at their fastest
>>> speed at boot time, and therefore loops_per_jiffy will be calibrated
>>> such that we guarantee _at least_ the asked-for delay - which is the
>>> only guarantee udelay has.
>> Even if the boot assumption is true, cpufreq actively changes the
>> loops_per_jiffy value when it changes freq. So, this could still mess up
>> the _at least_ guarantee.
> 
> Actually, it doesn't on SMP - if you build the kernel with SMP enabled,
> the code which fiddles with loops_per_jiffy is disabled.  See the
> #ifndef wrapping around adjust_jiffies() in drivers/cpufreq/cpufreq.c.

My comment above was for the non-SMP case (it was a reply to your 
comment about non-SMP case). In non-SMP case, cpufreq changes LPJ and 
the freq switch can happen while udelay is looping. That would mess up 
the minimum delay guarantee of udelay.

I was aware that cpufreq doesn't change LPJ for SMP. But I think they do 
that because they don't know where the arch specific per-CPU 
loops_per_jiffy is located. They expect the cpufreq driver to do the lpj 
scaling. So, per-CPU lpj is still going to change. At least, that's what 
I took out of the following comment:

/*
  * This function alters the system "loops_per_jiffy" for the clock
  * speed change.  Note that loops_per_jiffy cannot be updated on SMP
  * systems as each CPU might be scaled differently. So, use the arch
  * per-CPU loops_per_jiffy value wherever possible.
  */


> So, on SMP with cpufreq, the global loops_per_jiffy is a fixed value.
> Provided it was calibrated with the CPU running at max clock rate,
> the guarantee is satisfied for all CPUs in the system.

As mentioned earlier, I think the cpufreq driver for that specific arch 
is supposed to handle the LPJ changes. But let's assume that's not true. 
So, wouldn't this still be a problem? You could be doing udelay as if 
you are running at 1 GHz but you are actually running at 100 MHz. I 
would think that would be bad for performance and power (wasting cycles 
without going into WFI, etc).

Thanks,
Saravana

  reply	other threads:[~2010-04-21 23:47 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-04-21  2:19 udelay() broken for SMP cores? Saravana Kannan
2010-04-21  4:56 ` Shilimkar, Santosh
2010-04-21  6:43   ` Saravana Kannan
2010-04-21  7:22     ` Russell King - ARM Linux
2010-04-21  9:39       ` skannan at codeaurora.org
2010-04-21  9:50         ` Russell King - ARM Linux
2010-04-21  9:58           ` Gilles Chanteperdrix
2010-04-21 10:00           ` Jamie Lokier
2010-04-21 19:29             ` Russell King - ARM Linux
2010-04-21 19:52               ` Jamie Lokier
2010-04-21 20:21                 ` Russell King - ARM Linux
2010-04-21 20:47                   ` Jamie Lokier
2010-04-21 20:57                     ` Russell King - ARM Linux
2010-04-22  0:14                       ` Jamie Lokier
2011-01-08 23:24                         ` Russell King - ARM Linux
2010-04-21 10:31           ` skannan at codeaurora.org
2010-04-21 19:33             ` Russell King - ARM Linux
2010-04-21 23:47               ` Saravana Kannan [this message]
2010-04-21 23:47                 ` Saravana Kannan
2010-04-23  9:00           ` Pavel Machek

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=4BCF8E81.4080906@codeaurora.org \
    --to=skannan@codeaurora.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=santosh.shilimkar@ti.com \
    /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.