| Message ID | 1236723118-3577-1-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Timur Tabi wrote: > The macro spin_event_timeout() takes a condition and timeout value > (in microseconds) as parameters. It spins until either the condition is true > or the timeout expires. It returns zero if the timeout expires first, non-zero > otherwise. > > This primary purpose of this macro is to poll on a hardware register until a > status bit changes. The timeout ensures that the loop still terminates if the > bit doesn't change as expected. This macro makes it easier for driver > developers to perform this kind of operation properly. > > Signed-off-by: Timur Tabi <timur@freescale.com> > --- > > v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay Why make it powerpc-specific? This would be nice to have in arch-independent code. -Scott
On Tue, Mar 10, 2009 at 05:33:08PM -0500, Scott Wood wrote: > Timur Tabi wrote: >> The macro spin_event_timeout() takes a condition and timeout value >> (in microseconds) as parameters. It spins until either the condition is true >> or the timeout expires. It returns zero if the timeout expires first, non-zero >> otherwise. >> >> This primary purpose of this macro is to poll on a hardware register until a >> status bit changes. The timeout ensures that the loop still terminates if the >> bit doesn't change as expected. This macro makes it easier for driver >> developers to perform this kind of operation properly. >> >> Signed-off-by: Timur Tabi <timur@freescale.com> >> --- >> >> v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay > > Why make it powerpc-specific? This would be nice to have in > arch-independent code. That's just mean. He already posted it to lkml and was told to make it powerpc specific by Alan. josh
Josh Boyer wrote: > On Tue, Mar 10, 2009 at 05:33:08PM -0500, Scott Wood wrote: >> Timur Tabi wrote: >>> The macro spin_event_timeout() takes a condition and timeout value >>> (in microseconds) as parameters. It spins until either the condition is true >>> or the timeout expires. It returns zero if the timeout expires first, non-zero >>> otherwise. >>> >>> This primary purpose of this macro is to poll on a hardware register until a >>> status bit changes. The timeout ensures that the loop still terminates if the >>> bit doesn't change as expected. This macro makes it easier for driver >>> developers to perform this kind of operation properly. >>> >>> Signed-off-by: Timur Tabi <timur@freescale.com> >>> --- >>> >>> v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay >> Why make it powerpc-specific? This would be nice to have in >> arch-independent code. > > That's just mean. He already posted it to lkml and was told to make it > powerpc specific by Alan. Well, that's what happens when a discussion hops mailing lists with no backreference. :-P I don't see anywhere where he says it should be architecture dependent, but rather a general "I don't like this, get off my lawn!" response. I cannot agree with the "we shouldn't be encouraging this" sentiment; people don't generally do spin loops because they're lazy[1], but rather because the hardware demands it -- and it's hardly only on powerpc (much less just "some Freescale drivers") that I've encountered hardware that demands it, typiclally during reset/initialization or similarly non-hot paths. Why not provide something less likely to have bugs (the timeout case is unlikely to be well tested), more easily seen when reviewing a patch, and more likely to result in spin loops *with* a timeout rather than without? -Scott [1] Or rather, those that do should be smacked down during patch review.
On Tue, 2009-03-10 at 17:11 -0500, Timur Tabi wrote: > The macro spin_event_timeout() takes a condition and timeout value > (in microseconds) as parameters. It spins until either the condition is true > or the timeout expires. It returns zero if the timeout expires first, non-zero > otherwise. > > This primary purpose of this macro is to poll on a hardware register until a > status bit changes. The timeout ensures that the loop still terminates if the > bit doesn't change as expected. This macro makes it easier for driver > developers to perform this kind of operation properly. I've missed the history here but why is that arch code ? it could be used by more drivers... though there's always the idea that spinning is bad :-) Cheers, Ben. > Signed-off-by: Timur Tabi <timur@freescale.com> > --- > > v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay > > v4: removed cpu_relax (redundant), changed timeout to unsigned long > > v3: eliminated secondary evaluation of condition, replaced jiffies with udelay > > v2: added cpu_relax and time_before > > arch/powerpc/include/asm/delay.h | 32 ++++++++++++++++++++++++++++++++ > 1 files changed, 32 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/asm/delay.h > index f9200a6..aadec70 100644 > --- a/arch/powerpc/include/asm/delay.h > +++ b/arch/powerpc/include/asm/delay.h > @@ -2,6 +2,8 @@ > #define _ASM_POWERPC_DELAY_H > #ifdef __KERNEL__ > > +#include <asm/time.h> > + > /* > * Copyright 1996, Paul Mackerras. > * > @@ -30,5 +32,35 @@ extern void udelay(unsigned long usecs); > #define mdelay(n) udelay((n) * 1000) > #endif > > +/** > + * spin_event_timeout - spin until a condition gets true or a timeout elapses > + * @condition: a C expression to evalate > + * @timeout: timeout, in microseconds > + * > + * The process spins until the @condition evaluates to true (non-zero) or > + * the @timeout elapses. > + * > + * This primary purpose of this macro is to poll on a hardware register > + * until a status bit changes. The timeout ensures that the loop still > + * terminates if the bit never changes. > + * > + * The return value is non-zero if the condition evaluates to true first, or > + * zero if the timeout elapses first. > + */ > +#define spin_event_timeout(condition, timeout) \ > +({ \ > + unsigned long __start = get_tbl(); \ > + unsigned long __loops = tb_ticks_per_usec * timeout; \ > + int __ret = 1; \ > + while (!(condition)) { \ > + if (tb_ticks_since(__start) > __loops) { \ > + __ret = 0; \ > + break; \ > + } \ > + cpu_relax(); \ > + } \ > + __ret; \ > +}) > + > #endif /* __KERNEL__ */ > #endif /* _ASM_POWERPC_DELAY_H */
On Tue, 2009-03-10 at 18:37 -0400, Josh Boyer wrote: > On Tue, Mar 10, 2009 at 05:33:08PM -0500, Scott Wood wrote: > > Timur Tabi wrote: > >> The macro spin_event_timeout() takes a condition and timeout value > >> (in microseconds) as parameters. It spins until either the condition is true > >> or the timeout expires. It returns zero if the timeout expires first, non-zero > >> otherwise. > >> > >> This primary purpose of this macro is to poll on a hardware register until a > >> status bit changes. The timeout ensures that the loop still terminates if the > >> bit doesn't change as expected. This macro makes it easier for driver > >> developers to perform this kind of operation properly. > >> > >> Signed-off-by: Timur Tabi <timur@freescale.com> > >> --- > >> > >> v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay > > > > Why make it powerpc-specific? This would be nice to have in > > arch-independent code. > > That's just mean. He already posted it to lkml and was told to make it > powerpc specific by Alan. And ? We can disagree with Alan... Ben.
On Tue, Mar 10, 2009 at 6:59 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> And ? We can disagree with Alan...
If you guys want to argue with Alan on lkml, please go ahead. I could
use the support.
Alan did have one valid point though. Determining how long to loop
for is architecture-specific. Using jiffies is bad, because even one
jiffy is too long. Adding a udelay() inside the loop means that it
only checks he condition every microsecond. So the real solution is
to use keep looping until a certain amount of time has passed. This
means using an architecture-specific timebase register.
Now we can create a generic version of the function that uses jiffies,
and then arch-specific versions where possible. But Alan still needs
to be convinced. I already posted a length rebuttal to his email, but
I haven't gotten a reply yet.
On Tue, 2009-03-10 at 19:22 -0500, Timur Tabi wrote: > > Alan did have one valid point though. Determining how long to loop > for is architecture-specific. Using jiffies is bad, because even one > jiffy is too long. Adding a udelay() inside the loop means that it > only checks he condition every microsecond. So the real solution is > to use keep looping until a certain amount of time has passed. This > means using an architecture-specific timebase register. > Now we can create a generic version of the function that uses jiffies, > and then arch-specific versions where possible. But Alan still needs > to be convinced. I already posted a length rebuttal to his email, but > I haven't gotten a reply yet. > There are several aspects here: - The amount of time to wait should be specified by the caller since it's generally going to come from HW specs - The amount of time between the polls ... that could also be an argument to the macro, not sure there - The precision of the actual wait calls... I vote for microseconds for everything and udelay. The arch will do its best. Cheers, Ben.
On Tue, Mar 10, 2009 at 05:58:58PM -0500, Scott Wood wrote: > Josh Boyer wrote: >> On Tue, Mar 10, 2009 at 05:33:08PM -0500, Scott Wood wrote: >>> Timur Tabi wrote: >>>> The macro spin_event_timeout() takes a condition and timeout value >>>> (in microseconds) as parameters. It spins until either the condition is true >>>> or the timeout expires. It returns zero if the timeout expires first, non-zero >>>> otherwise. >>>> >>>> This primary purpose of this macro is to poll on a hardware register until a >>>> status bit changes. The timeout ensures that the loop still terminates if the >>>> bit doesn't change as expected. This macro makes it easier for driver >>>> developers to perform this kind of operation properly. >>>> >>>> Signed-off-by: Timur Tabi <timur@freescale.com> >>>> --- >>>> >>>> v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay >>> Why make it powerpc-specific? This would be nice to have in >>> arch-independent code. >> >> That's just mean. He already posted it to lkml and was told to make it >> powerpc specific by Alan. > > Well, that's what happens when a discussion hops mailing lists with no > backreference. :-P > > I don't see anywhere where he says it should be architecture dependent, > but rather a general "I don't like this, get off my lawn!" response. > > I cannot agree with the "we shouldn't be encouraging this" sentiment; > people don't generally do spin loops because they're lazy[1], but rather > because the hardware demands it -- and it's hardly only on powerpc (much > less just "some Freescale drivers") that I've encountered hardware that > demands it, typiclally during reset/initialization or similarly non-hot > paths. Why not provide something less likely to have bugs (the timeout > case is unlikely to be well tested), more easily seen when reviewing a > patch, and more likely to result in spin loops *with* a timeout rather > than without? Excellent questions. Did you send them to lkml and Alan? josh
On Wed, Mar 11, 2009 at 10:59:11AM +1100, Benjamin Herrenschmidt wrote: >On Tue, 2009-03-10 at 18:37 -0400, Josh Boyer wrote: >> On Tue, Mar 10, 2009 at 05:33:08PM -0500, Scott Wood wrote: >> > Timur Tabi wrote: >> >> The macro spin_event_timeout() takes a condition and timeout value >> >> (in microseconds) as parameters. It spins until either the condition is true >> >> or the timeout expires. It returns zero if the timeout expires first, non-zero >> >> otherwise. >> >> >> >> This primary purpose of this macro is to poll on a hardware register until a >> >> status bit changes. The timeout ensures that the loop still terminates if the >> >> bit doesn't change as expected. This macro makes it easier for driver >> >> developers to perform this kind of operation properly. >> >> >> >> Signed-off-by: Timur Tabi <timur@freescale.com> >> >> --- >> >> >> >> v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay >> > >> > Why make it powerpc-specific? This would be nice to have in >> > arch-independent code. >> >> That's just mean. He already posted it to lkml and was told to make it >> powerpc specific by Alan. > >And ? We can disagree with Alan... Did I say Alan was right? I'm just explaining why Timur probably posted it as arch-specific. josh
> Alan did have one valid point though. Determining how long to loop > for is architecture-specific. Using jiffies is bad, because even one > jiffy is too long. Adding a udelay() inside the loop means that it > only checks he condition every microsecond. So the real solution is > to use keep looping until a certain amount of time has passed. This > means using an architecture-specific timebase register. Are there really cases where spinning for 1 jiffy is too long of a timeout? It might make sense for the parameter passed in to be in terms of microseconds but I have a hard time coming up with a case where having the real timeout be 40 msecs or whatever 1 jiffy ends up being is a real problem -- after all, this helper is intended for the case where we expect the condition to become true much sooner than the worst case. - R.
On Wed, Mar 11, 2009 at 12:09 AM, Roland Dreier <rdreier@cisco.com> wrote: > Are there really cases where spinning for 1 jiffy is too long of a > timeout? If the result is a timeout, then I say no. A timeout is an error condition, and the code will usually terminate. > It might make sense for the parameter passed in to be in terms > of microseconds but I have a hard time coming up with a case where > having the real timeout be 40 msecs or whatever 1 jiffy ends up being is > a real problem -- after all, this helper is intended for the case where > we expect the condition to become true much sooner than the worst case. Well, that's the point. What if the condition takes a long time to come true. One argument against this code is that it encourages developers to use busy-waits for long periods of time. The only way to prevent this is to make the timeout really short. But if we're using jiffies, then the minimum amount of time needs to be two. It can't be one, because what if jiffies increments immediately after starting the loop? So you need to use a value of two as a minimum. Two jiffies can be a very long time. Besides, if this function is used when interrupts are disabled, I believe that on some platforms, jiffies never increments. If so, we can't use the actual 'jiffies' variable.
Timur Tabi wrote: > On Wed, Mar 11, 2009 at 12:09 AM, Roland Dreier <rdreier@cisco.com> wrote: > >> Are there really cases where spinning for 1 jiffy is too long of a >> timeout? > > If the result is a timeout, then I say no. A timeout is an error > condition, and the code will usually terminate. [snip] > Two jiffies can be a very long time. One jiffy is fine, but two is just too long? Given that it only happens in cases of malfunctioning hardware (or a buggy driver), it seems reasonable as long as preemption isn't disabled (I'm assuming anyone that cares about a rare latency of a couple jiffies is using a preemptible kernel). > Besides, if this function is > used when interrupts are disabled, I believe that on some platforms, > jiffies never increments. If so, we can't use the actual 'jiffies' > variable. Disallow that, enforced with a call to might_sleep(). Alternatively, do something with get_cycles(), and have some sort of #define by which arches can say if get_cycles actually works. In the absence of a working get_cycles() or equivalent, timeouts with interrupts disabled aren't going to happen whether we abstract it with a macro or not. -Scott
On Tue, Mar 10, 2009 at 6:24 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Tue, 2009-03-10 at 19:22 -0500, Timur Tabi wrote: >> >> Alan did have one valid point though. Determining how long to loop >> for is architecture-specific. Using jiffies is bad, because even one >> jiffy is too long. Adding a udelay() inside the loop means that it >> only checks he condition every microsecond. So the real solution is >> to use keep looping until a certain amount of time has passed. This >> means using an architecture-specific timebase register. > >> Now we can create a generic version of the function that uses jiffies, >> and then arch-specific versions where possible. But Alan still needs >> to be convinced. I already posted a length rebuttal to his email, but >> I haven't gotten a reply yet. >> > There are several aspects here: > > - The amount of time to wait should be specified by the caller since > it's generally going to come from HW specs > > - The amount of time between the polls ... that could also be an > argument to the macro, not sure there > > - The precision of the actual wait calls... I vote for microseconds for > everything and udelay. The arch will do its best. No, not udelay. Or any delay for that matter. If spinning on a condition, then there is no advantage to burning cycles with a udelay(). Those cycles may as well be used to keep testing the condition so the loop can be exited faster. a udelay() would only serve to always make the busywait longer. g.
On Wed, Mar 11, 2009 at 11:51 AM, Scott Wood <scottwood@freescale.com> wrote: > One jiffy is fine, but two is just too long? Any number of jiffies is *not* too long if a timeout occurs. However, I think even one jiffy is too long if that's the normal condition. Unfortunately, the driver may not have any choice in some circumstances. If the hardware is just too slow to respond, and it doesn't provide interrupts, but the code is running in atomic context, and the function what else can it do? > Disallow that, enforced with a call to might_sleep(). I think we need to be able to allow this function to work in atomic context. Is jiffies updated in atomic context? > Alternatively, do something with get_cycles(), and have some sort of #define > by which arches can say if get_cycles actually works. In the absence of a > working get_cycles() or equivalent, timeouts with interrupts disabled aren't > going to happen whether we abstract it with a macro or not. I think I can live with that.
Timur Tabi wrote: > On Wed, Mar 11, 2009 at 11:51 AM, Scott Wood <scottwood@freescale.com> wrote: > >> One jiffy is fine, but two is just too long? > > Any number of jiffies is *not* too long if a timeout occurs. However, > I think even one jiffy is too long if that's the normal condition. I was under the impression that we were only talking about timeouts, and that the common case was significantly shorter than that. > Unfortunately, the driver may not have any choice in some > circumstances. If the hardware is just too slow to respond, and it > doesn't provide interrupts, but the code is running in atomic context, > and the function what else can it do? Rework the driver to poll from a periodic timer (like we do with PHYs). However, that's overkill when the hardware is supposed to respond in a handful of clocks, and preemption is enabled in case the timeout path does happen. >> Disallow that, enforced with a call to might_sleep(). > > I think we need to be able to allow this function to work in atomic > context. Is jiffies updated in atomic context? If it's atomic because preemption was disabled, yes -- but even a rare extended spin in such a context would be bad for hard realtime. If interrupts are disabled, or the code is executing from a timer interrupt (or possibly other interrupts depending on the hardware and its priority scheme), no. >> Alternatively, do something with get_cycles(), and have some sort of #define >> by which arches can say if get_cycles actually works. In the absence of a >> working get_cycles() or equivalent, timeouts with interrupts disabled aren't >> going to happen whether we abstract it with a macro or not. > > I think I can live with that. Another option is to use udelay() on platforms without a working get_cycles(). -Scott
On Wed, Mar 11, 2009 at 2:22 PM, Scott Wood <scottwood@freescale.com> wrote: > I was under the impression that we were only talking about timeouts, and > that the common case was significantly shorter than that. I think one of the concerns that Alan Cox raised is that the existence of this macro would encourage people to spin for long durations. > If it's atomic because preemption was disabled, yes -- but even a rare > extended spin in such a context would be bad for hard realtime. If > interrupts are disabled, or the code is executing from a timer interrupt (or > possibly other interrupts depending on the hardware and its priority > scheme), no. So in that case, I can't rely on jiffies. I guess get_cycle() is my only choice. The problem is that there is no num_cycles_per_usec().
Timur Tabi wrote: > On Wed, Mar 11, 2009 at 2:22 PM, Scott Wood <scottwood@freescale.com> wrote: > >> I was under the impression that we were only talking about timeouts, and >> that the common case was significantly shorter than that. > > I think one of the concerns that Alan Cox raised is that the existence > of this macro would encourage people to spin for long durations. Yes, and I've already stated my response to that line of thinking. I just don't see anyone who would have done something better than a spin loop changing their mind because doing a spin loop becomes a little easier -- the spin loop is *already* easier than the alternatives. What if another variant were added that did msleep between iterations, for longer expected completion times? Or if we want to be really fancy, combine them into one function that starts with small udelays, then switches to msleep of progressively larger intervals up to some maximum? >> If it's atomic because preemption was disabled, yes -- but even a rare >> extended spin in such a context would be bad for hard realtime. If >> interrupts are disabled, or the code is executing from a timer interrupt (or >> possibly other interrupts depending on the hardware and its priority >> scheme), no. > > So in that case, I can't rely on jiffies. Or you can say that atomic context is outside the scope of this macro. -Scott
Scott Wood wrote:
> Or you can say that atomic context is outside the scope of this macro.
No, I don't want to say that. We have wait_event_timeout() for larger-scale
operations. I'm just looking for something that can replace "while (!condition);"
Timur Tabi wrote: > Scott Wood wrote: > > > Or you can say that atomic context is outside the scope of this macro. > > No, I don't want to say that. We have wait_event_timeout() for > larger-scale operations. I'm just looking for something that can > replace "while (!condition);" wait_event_timeout() requires a wait queue. -Scott
> No, not udelay. Or any delay for that matter. If spinning on a > condition, then there is no advantage to burning cycles with a > udelay(). Those cycles may as well be used to keep testing the > condition so the loop can be exited faster. a udelay() would only > serve to always make the busywait longer. Well, there's a non-empty set of HW where polling as fast as you can will effectively prevent it to make fwd progress... Ben.
Benjamin Herrenschmidt wrote: > Well, there's a non-empty set of HW where polling as fast as you can > will effectively prevent it to make fwd progress... Alan Cox mentioned this. He gave PCI and 10us as an example. I suggested adding a third parameter that would be a udelay() inserted into the loop. He countered with this: spin_until_timeout(readb(foo) & 0x80, 30 * HZ) { udelay(10); /* Maybe do other stuff */ } But I don't know how to make that work *and* have it return a value indicating timeout or success.
Timur Tabi wrote: > Benjamin Herrenschmidt wrote: > >> Well, there's a non-empty set of HW where polling as fast as you can >> will effectively prevent it to make fwd progress... > > Alan Cox mentioned this. He gave PCI and 10us as an example. I > suggested adding a third parameter that would be a udelay() inserted > into the loop. He countered with this: > > spin_until_timeout(readb(foo) & 0x80, 30 * HZ) { > udelay(10); > /* Maybe do other stuff */ > } Hmm, the person objecting that it could lead to people using it for excessive timeouts suggested a timeout of *30 seconds*? > But I don't know how to make that work *and* have it return a value > indicating timeout or success. And it also doesn't allow using the udelay as part of the timeout mechanism. -Scott
diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/asm/delay.h index f9200a6..aadec70 100644 --- a/arch/powerpc/include/asm/delay.h +++ b/arch/powerpc/include/asm/delay.h @@ -2,6 +2,8 @@ #define _ASM_POWERPC_DELAY_H #ifdef __KERNEL__ +#include <asm/time.h> + /* * Copyright 1996, Paul Mackerras. * @@ -30,5 +32,35 @@ extern void udelay(unsigned long usecs); #define mdelay(n) udelay((n) * 1000) #endif +/** + * spin_event_timeout - spin until a condition gets true or a timeout elapses + * @condition: a C expression to evalate + * @timeout: timeout, in microseconds + * + * The process spins until the @condition evaluates to true (non-zero) or + * the @timeout elapses. + * + * This primary purpose of this macro is to poll on a hardware register + * until a status bit changes. The timeout ensures that the loop still + * terminates if the bit never changes. + * + * The return value is non-zero if the condition evaluates to true first, or + * zero if the timeout elapses first. + */ +#define spin_event_timeout(condition, timeout) \ +({ \ + unsigned long __start = get_tbl(); \ + unsigned long __loops = tb_ticks_per_usec * timeout; \ + int __ret = 1; \ + while (!(condition)) { \ + if (tb_ticks_since(__start) > __loops) { \ + __ret = 0; \ + break; \ + } \ + cpu_relax(); \ + } \ + __ret; \ +}) + #endif /* __KERNEL__ */ #endif /* _ASM_POWERPC_DELAY_H */
The macro spin_event_timeout() takes a condition and timeout value (in microseconds) as parameters. It spins until either the condition is true or the timeout expires. It returns zero if the timeout expires first, non-zero otherwise. This primary purpose of this macro is to poll on a hardware register until a status bit changes. The timeout ensures that the loop still terminates if the bit doesn't change as expected. This macro makes it easier for driver developers to perform this kind of operation properly. Signed-off-by: Timur Tabi <timur@freescale.com> --- v5: ported to arch/powerpc, made it powerpc-specific, eliminated udelay v4: removed cpu_relax (redundant), changed timeout to unsigned long v3: eliminated secondary evaluation of condition, replaced jiffies with udelay v2: added cpu_relax and time_before arch/powerpc/include/asm/delay.h | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)