[PATCH v5] introduce macro spin_event_timeout()

[PATCH v5] introduce macro spin_event_timeout()

[Timur Tabi]

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(-)

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 */
-- 
1.6.1.3

Re: [PATCH v5] introduce macro spin_event_timeout()

[Scott Wood]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Josh Boyer]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Scott Wood]

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.

Re: [PATCH v5] introduce macro spin_event_timeout()

[Benjamin Herrenschmidt]

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 */

Re: [PATCH v5] introduce macro spin_event_timeout()

[Benjamin Herrenschmidt]

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.

Re: [PATCH v5] introduce macro spin_event_timeout()

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH v5] introduce macro spin_event_timeout()

[Benjamin Herrenschmidt]

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.

Re: [PATCH v5] introduce macro spin_event_timeout()

[Josh Boyer]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Josh Boyer]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Roland Dreier]

 > 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.

Re: [PATCH v5] introduce macro spin_event_timeout()

[Timur Tabi]

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
Linux kernel developer at Freescale

Re: [PATCH v5] introduce macro spin_event_timeout()

[Scott Wood]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Grant Likely]

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. =A0Determining how long to loop
>> for is architecture-specific. =A0Using jiffies is bad, because even one
>> jiffy is too long. =A0Adding a udelay() inside the loop means that it
>> only checks he condition every microsecond. =A0So the real solution is
>> to use keep looping until a certain amount of time has passed. =A0This
>> 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. =A0But Alan still needs
>> to be convinced. =A0I already posted a length rebuttal to his email, but
>> I haven't gotten a reply yet.
>>
> There are several aspects here:
>
> =A0- The amount of time to wait should be specified by the caller since
> it's generally going to come from HW specs
>
> =A0- The amount of time between the polls ... that could also be an
> argument to the macro, not sure there
>
> =A0- The precision of the actual wait calls... I vote for microseconds fo=
r
> 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.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v5] introduce macro spin_event_timeout()

[Timur Tabi]

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
Linux kernel developer at Freescale

Re: [PATCH v5] introduce macro spin_event_timeout()

[Scott Wood]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Timur Tabi]

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. =A0If
> 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().

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH v5] introduce macro spin_event_timeout()

[Scott Wood]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Timur Tabi]

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
Linux Kernel Developer @ Freescale

Re: [PATCH v5] introduce macro spin_event_timeout()

[Scott Wood]

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

Re: [PATCH v5] introduce macro spin_event_timeout()

[Benjamin Herrenschmidt]

> 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.

Re: [PATCH v5] introduce macro spin_event_timeout()

[Timur Tabi]

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
Linux Kernel Developer @ Freescale

Re: [PATCH v5] introduce macro spin_event_timeout()

[Scott Wood]

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