Re: yield() in i2c non-happy paths hits BUG under -rt patch
On Wed, 18 Nov 2009 21:36:48 +0100 (CET), Thomas Gleixner wrote: On Wed, 18 Nov 2009, Jean Delvare wrote: On Wed, 18 Nov 2009 17:28:53 +0100, Leon Woestenberg wrote: On Wed, Nov 18, 2009 at 2:05 AM, Alan Cox a...@lxorguk.ukuu.org.uk wrote: Our timers are very efficient and some day we will need to make jiffies a function and stop the timer ticking for best performance. At that point timers are probably the most efficient way to do much of this. The problem with I2C bitbanged is the stringent timing, we need a way to have fine-grained sleeping mixed with real-time tasks in order to make this work. FWIW, the problem that was initially reported has nothing to do with this. i2c-algo-bit used mdelay() during transactions, not yield(). yield() is used only in once place, _between_ transactions attempts. There are no strict timing constraints there. That still does not explain why yield() is necessary _between_ the transaction attempts. It is not _necessary_. We are just trying to be fair to other kernel threads, because bit-banging is expensive and this is the only case where we know we can tolerate a delay. Just to clarify things... does (or did) yield() have anything to do with CONFIG_PREEMPT_VOLUNTARY? That code is fully preemptible, otherwise you could not call yield(). How does one know what code is preemtible and what code is not? The rest of the i2c-algo-bit code should definitely _not_ be preemtible, as it is highly timing sensitive. And as I said before nobody even noticed that the yield() default implementation was changed to a NOOP by default in the scheduler. Well, I guess only people monitoring system latency would notice, as this is the only thing yield() was supposed to help with in the first place. You say NOOP by default, does this imply there is a way to change this? Was yield() turned into NOOP by design, or was it a bug? -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
Well, I guess only people monitoring system latency would notice, as this is the only thing yield() was supposed to help with in the first place. if (need_resched()) schedule(); will make non-rt tasks act politely at the right moments. RT tasks will likely immediately get to take the CPU again depending upon the scheduling parameters in use. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
On Thu, 2009-11-19 at 12:59 +, Alan Cox wrote: Well, I guess only people monitoring system latency would notice, as this is the only thing yield() was supposed to help with in the first place. if (need_resched()) schedule(); aka. cond_resched(); will make non-rt tasks act politely at the right moments. RT tasks will likely immediately get to take the CPU again depending upon the scheduling parameters in use. Right, FIFO will simply NOP it, since if it was the highest running task, it will still be. RR could possibly run out of its slice and schedule to another RR of the same prio. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
On Thu, 2009-11-19 at 14:11 +0100, Thomas Gleixner wrote: You say NOOP by default, does this imply there is a way to change this? There is a sysctl: sysctl_sched_compat_yield This makes yield() place current behind all other tasks, and sucks too for some workloads. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
Hi Peter, On Thu, 19 Nov 2009 14:06:54 +0100, Peter Zijlstra wrote: On Thu, 2009-11-19 at 12:59 +, Alan Cox wrote: Well, I guess only people monitoring system latency would notice, as this is the only thing yield() was supposed to help with in the first place. if (need_resched()) schedule(); aka. cond_resched(); Are you saying that most calls to yield() should be replaced with calls to cond_resched()? I admit I a little skeptical. While the description of cond_resched() (latency reduction via explicit rescheduling in places that are safe) sounds promising, following the calls leads me to: static inline int need_resched(void) { return unlikely(test_thread_flag(TIF_NEED_RESCHED)); } So apparently the condition for need_resched() to do anything is considered unlikely... suggesting that cond_resched() is a no-op in most cases? I don't quite get the point of moving away from sched() because it is a no-op, if we end up with a no-op under a different name. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
On Thu, 2009-11-19 at 15:00 +0100, Jean Delvare wrote: cond_resched(); Are you saying that most calls to yield() should be replaced with calls to cond_resched()? No, depends on the reason yield() is used. Some cases can be replaced by locking constructs, such as a condition variable. I admit I a little skeptical. While the description of cond_resched() (latency reduction via explicit rescheduling in places that are safe) sounds promising, following the calls leads me to: static inline int need_resched(void) { return unlikely(test_thread_flag(TIF_NEED_RESCHED)); } So apparently the condition for need_resched() to do anything is considered unlikely... suggesting that cond_resched() is a no-op in most cases? I don't quite get the point of moving away from sched() because it is a no-op, if we end up with a no-op under a different name. TIF_NEED_RESCHED gets set by the scheduler whenever it decides current needs to get preempted, its unlikely() because that reduces the code impact of cond_resched() and similar in the case we don't schedule, if we do schedule() a mis-predicted branch isn't going to be noticed on the overhead of scheduling. So there's a few cases, 1) PREEMPT=n 2) Voluntary preempt 3) PREEMPT=y 1) non of this has any effect, if the scheduler wants to reschedule a task that's in the kernel, it'll have to wait until it gets back to user-space. 2) uses cond_resched() and similar to have explicit preemption points, so we don't need to wait as long as 1). 3) preempts directly when !preempt_count(), when IRQs are disabled, the IPI that will accompany TIF_NEED_RESCHED will be delayed and local_irq_enable()/restore() will effect a reschedule due to the pending IPI. If preemption was disabled while the IPI hit nothing will happen, but preempt_enable() will do the reschedule once preempt_count reaches 0. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
Hello, On Wed, Nov 18, 2009 at 2:05 AM, Alan Cox a...@lxorguk.ukuu.org.uk wrote: I think the yield()s in the device driver code means I need a small delay before the hardware is ready which might translate to some arbitrary let me msleep() or do not select this task in the next scheduler run, EVEN IF this task is highest priority. Yield() in a driver is almost always a bug. I know and that's exactly why I started this thread (and of course, because I ran into the bug on my system). Our timers are very efficient and some day we will need to make jiffies a function and stop the timer ticking for best performance. At that point timers are probably the most efficient way to do much of this. The problem with I2C bitbanged is the stringent timing, we need a way to have fine-grained sleeping mixed with real-time tasks in order to make this work. As Thomas already said, the hardware is broken (in the sense that I2C should really rely on hardware timers, i.e. an I2C host controller). However, much of the cheaper/older/... embedded hardware is broken. Given that I2C devices are relatively easy on the timing, we need the least-dirty way that is not buggy in the kernel. Be that as it may, yield() in a driver is almost always the wrong thing to do. Yes. What is your idea on removing those without breaking functionality? Fine-graining sleep()ing? Regards, -- Leon -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
On Wed, 18 Nov 2009 17:28:53 +0100, Leon Woestenberg wrote: On Wed, Nov 18, 2009 at 2:05 AM, Alan Cox a...@lxorguk.ukuu.org.uk wrote: Our timers are very efficient and some day we will need to make jiffies a function and stop the timer ticking for best performance. At that point timers are probably the most efficient way to do much of this. The problem with I2C bitbanged is the stringent timing, we need a way to have fine-grained sleeping mixed with real-time tasks in order to make this work. FWIW, the problem that was initially reported has nothing to do with this. i2c-algo-bit used mdelay() during transactions, not yield(). yield() is used only in once place, _between_ transactions attempts. There are no strict timing constraints there. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
On Wed, 18 Nov 2009, Jean Delvare wrote: On Wed, 18 Nov 2009 17:28:53 +0100, Leon Woestenberg wrote: On Wed, Nov 18, 2009 at 2:05 AM, Alan Cox a...@lxorguk.ukuu.org.uk wrote: Our timers are very efficient and some day we will need to make jiffies a function and stop the timer ticking for best performance. At that point timers are probably the most efficient way to do much of this. The problem with I2C bitbanged is the stringent timing, we need a way to have fine-grained sleeping mixed with real-time tasks in order to make this work. FWIW, the problem that was initially reported has nothing to do with this. i2c-algo-bit used mdelay() during transactions, not yield(). yield() is used only in once place, _between_ transactions attempts. There are no strict timing constraints there. That still does not explain why yield() is necessary _between_ the transaction attempts. That code is fully preemptible, otherwise you could not call yield(). And as I said before nobody even noticed that the yield() default implementation was changed to a NOOP by default in the scheduler. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
I think the yield()s in the device driver code means I need a small delay before the hardware is ready which might translate to some arbitrary let me msleep() or do not select this task in the next scheduler run, EVEN IF this task is highest priority. Yield() in a driver is almost always a bug. The reason for that is that doing do { inb(); } while(!something); which is what yield can end up as being if there is nothing else on that CPU is extremely bad for bus performance on most systems. It's almost always better to be using msleep() or even mdelay + a check to see if a reschedule is needed/schedule(). I assume this is rather dirty and has too much overhead on the timer interfaces. Our timers are very efficient and some day we will need to make jiffies a function and stop the timer ticking for best performance. At that point timers are probably the most efficient way to do much of this. Be that as it may, yield() in a driver is almost always the wrong thing to do. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
On Fri, Nov 13, 2009 at 11:03:39PM +0100, Thomas Gleixner wrote: Q: Why put people yield() into their code ? A: Because: - it is less nasty than busy waiting for a long time - it works better ... I can see the idea of using yield() to let other tasks making progress in situations where the hardware is a design failure as well, e.g. bitbang devices. But if we have to deal with hardware which is crap by design yield() is the worst of all answers simply due to its horrible semantics. What other options are there available for the first case (which is often why things work better with the use of yield) that don't involve sleeps, or is the idea that in situations like this drivers should always sleep? -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
Hi Thomas, On Fri, 13 Nov 2009 23:03:39 +0100 (CET), Thomas Gleixner wrote: Jean, On Thu, 12 Nov 2009, Jean Delvare wrote: As far as I can see, yield() doesn't have clear semantics attached. It's more of a utility function everyone could use as they see fit. In that respect, I understand your concerns about the coders' expectations and how they could be dismissed by RT. OTOH, I don't think that asking all developers to get rid of yield() because it _may not be_ RT-compliant will lead you anywhere. In the 3 occurrences I've looked at, yield() is used as a way to busy-wait in a multitask-friendly way. What other use cases do you expect? I've never seen yield() used as a way to avoid deadlocks, which seems to be what you fear. I guess it could statistically be used that way, but obviously I wouldn't recommend it. Anything else? I would recommend that you audit the various use cases of yield(), and then offer replacements for the cases which are RT-unfriendly, leaving the rest alone and possibly clarifying the intended use of yield() to avoid future problems. In the i2c-algo-bit case, which started this thread, I can't really see what something more explicit of an implementation would be. yield() is as optimum as you can get, only delaying the processing if other tasks want to run. A sleep or a delay would delay the processing unconditionally, for an arbitrary amount of time, with no good reason. Removing yield() would increase the latency. This particular feature of i2c-algo-bit isn't necessarily terribly useful, but the code does the right thing, regardless of RT, so I just can't see any reason to change it. The problem with yield() is not RT specific. Let's just look at the yield semantics in mainline: From kernel/sched_fair.c unsigned int __read_mostly sysctl_sched_compat_yield; ... static void yield_task_fair(struct rq *rq) { if (likely(!sysctl_sched_compat_yield) curr-policy != SCHED_BATCH) { ... return; } } So even in mainline yield() is doing nothing vs. latencies and is not multitask friendly by default. It's just not complaining about it. I admit I have no clue what you're talking about. What I see is: /** * yield - yield the current processor to other threads. * * This is a shortcut for kernel-space yielding - it marks the * thread runnable and calls sys_sched_yield(). */ void __sched yield(void) { set_current_state(TASK_RUNNING); sys_sched_yield(); } I have no clue where sys_sched_yield is defined, but I trust the comment as to what sched() is doing. yield itself is a design failure in almost all of its aspects. It's the poor mans replacement for proper async notification. All the use cases in the i2c subsystem have nothing to do with async notification. Q: Why put people yield() into their code ? A: Because: - it is less nasty than busy waiting for a long time That's what I've seen, yes. - it works better This is vague. - it solves the hang In which case it is definitely a design failure, granted. - it happened to be in the driver they copied - At least 90% of the in kernel users of yield() are either a bug or a design problem or lack of understanding or all of those. I can see the idea of using yield() to let other tasks making progress in situations where the hardware is a design failure as well, e.g. bitbang devices. But if we have to deal with hardware which is crap by design yield() is the worst of all answers simply due to its horrible semantics. Let's look at the code in question: for (i = 0; i = retries; i++) { ret = i2c_outb(i2c_adap, addr); if (ret == 1 || i == retries) break; bit_dbg(3, i2c_adap-dev, emitting stop condition\n); i2c_stop(adap); udelay(adap-udelay); yield(); We are in the error path as we failed to communicate with the device in the first place. So there a two scenarios: 1) the device is essential for the boot process: you have hit a problem anyway No, you have not. If you exhaust all the retries, then yes you have a problem. But if the first attempt fails and the second works, then all is fine. And this can happen. This is the whole point of the retry loop! 2) this is just some random device probing: who cares ? Who cares about what? Me, I certainly care about this probing not taking too much time, to not slow down the boot process for example (framebuffer drivers do such probes over the DDC bus.) On top of this, this may not be random device probing but a real access to a known device, which is busy and thus doesn't ack its address. This is permitted by I2C. A common case is I2C EEPROMs, which are busy when writing a page, and need some time before they will ack their address again. But it will
Re: yield() in i2c non-happy paths hits BUG under -rt patch
Jean, On Thu, 12 Nov 2009, Jean Delvare wrote: As far as I can see, yield() doesn't have clear semantics attached. It's more of a utility function everyone could use as they see fit. In that respect, I understand your concerns about the coders' expectations and how they could be dismissed by RT. OTOH, I don't think that asking all developers to get rid of yield() because it _may not be_ RT-compliant will lead you anywhere. In the 3 occurrences I've looked at, yield() is used as a way to busy-wait in a multitask-friendly way. What other use cases do you expect? I've never seen yield() used as a way to avoid deadlocks, which seems to be what you fear. I guess it could statistically be used that way, but obviously I wouldn't recommend it. Anything else? I would recommend that you audit the various use cases of yield(), and then offer replacements for the cases which are RT-unfriendly, leaving the rest alone and possibly clarifying the intended use of yield() to avoid future problems. In the i2c-algo-bit case, which started this thread, I can't really see what something more explicit of an implementation would be. yield() is as optimum as you can get, only delaying the processing if other tasks want to run. A sleep or a delay would delay the processing unconditionally, for an arbitrary amount of time, with no good reason. Removing yield() would increase the latency. This particular feature of i2c-algo-bit isn't necessarily terribly useful, but the code does the right thing, regardless of RT, so I just can't see any reason to change it. The problem with yield() is not RT specific. Let's just look at the yield semantics in mainline: From kernel/sched_fair.c unsigned int __read_mostly sysctl_sched_compat_yield; ... static void yield_task_fair(struct rq *rq) { if (likely(!sysctl_sched_compat_yield) curr-policy != SCHED_BATCH) { ... return; } } So even in mainline yield() is doing nothing vs. latencies and is not multitask friendly by default. It's just not complaining about it. yield itself is a design failure in almost all of its aspects. It's the poor mans replacement for proper async notification. Q: Why put people yield() into their code ? A: Because: - it is less nasty than busy waiting for a long time - it works better - it solves the hang - it happened to be in the driver they copied - At least 90% of the in kernel users of yield() are either a bug or a design problem or lack of understanding or all of those. I can see the idea of using yield() to let other tasks making progress in situations where the hardware is a design failure as well, e.g. bitbang devices. But if we have to deal with hardware which is crap by design yield() is the worst of all answers simply due to its horrible semantics. Let's look at the code in question: for (i = 0; i = retries; i++) { ret = i2c_outb(i2c_adap, addr); if (ret == 1 || i == retries) break; bit_dbg(3, i2c_adap-dev, emitting stop condition\n); i2c_stop(adap); udelay(adap-udelay); yield(); We are in the error path as we failed to communicate with the device in the first place. So there a two scenarios: 1) the device is essential for the boot process: you have hit a problem anyway 2) this is just some random device probing: who cares ? So in both cases the following change is a noop vs. latency and whatever your concerns are: - udelay(adap-udelay); - yield(); + msleep(1); Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
Hello Sven, On Sun, 08 Nov 2009 10:57:16 -0800, Sven-Thorsten Dietrich wrote: On 11/07/2009 12:01 PM, Jean Delvare wrote: One thing I do not understand: if yield() is a bug to RT kernels, then we would have to remove them all? But so far, yield() still exists in the kernel tree, and it serves a purpose. Are you going to ask all developers to remove all occurrences of yield() in their code? Doesn't sound terribly realistic. The flaw in using yield() with RT priorities, is that it doesn't do what you expect. You know, I did not really expect anything in the first place, given how little I know about RT ;) The scheduler will run, and pick the highest-priority thread, which is the one yield()-ing. Unless there are several real-time threads running, I presume? So the risk is, that whatever the yield() intended to do, it won't do, and worse, that you will end up endlessly yielding to yourself, locking the machine. So the call is for something more explicit of an implementation. This seem to imply an affirmative answer to my initial question: your plan is to get rid of the ~500 occurrences of yield() throughout the kernel tree? As far as I can see, yield() doesn't have clear semantics attached. It's more of a utility function everyone could use as they see fit. In that respect, I understand your concerns about the coders' expectations and how they could be dismissed by RT. OTOH, I don't think that asking all developers to get rid of yield() because it _may not be_ RT-compliant will lead you anywhere. In the 3 occurrences I've looked at, yield() is used as a way to busy-wait in a multitask-friendly way. What other use cases do you expect? I've never seen yield() used as a way to avoid deadlocks, which seems to be what you fear. I guess it could statistically be used that way, but obviously I wouldn't recommend it. Anything else? I would recommend that you audit the various use cases of yield(), and then offer replacements for the cases which are RT-unfriendly, leaving the rest alone and possibly clarifying the intended use of yield() to avoid future problems. In the i2c-algo-bit case, which started this thread, I can't really see what something more explicit of an implementation would be. yield() is as optimum as you can get, only delaying the processing if other tasks want to run. A sleep or a delay would delay the processing unconditionally, for an arbitrary amount of time, with no good reason. Removing yield() would increase the latency. This particular feature of i2c-algo-bit isn't necessarily terribly useful, but the code does the right thing, regardless of RT, so I just can't see any reason to change it. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
yield() in i2c non-happy paths hits BUG under -rt patch
Hello Jean, Ben, -i2c and -rt devs, during testing the Linux PREEMP_RT work (step-by-step being merged with mainline) together with I2C functionality I hit the fact that the I2C subsystem uses yield() in some of the non-happy code paths (mostly during chip / address probing etc). My (embedded) system was running a low-priority real-time work on the generic workqueue which tried to blink a LED using an I2C I/O multiplexer when I hit the BUG where this real-time task ran into the yield() of try_address(). Grepping through the I2C subsystem code there where more yield() sprinkled in, without it being clear to me why they are there. Can those yield()s please be removed, and if they are needed for some reason (??) be replaced with something equivalent? I am no longer with this particular project, I do not have the defconfig and kernel logs at my disposal. Regards, -- Leon -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: yield() in i2c non-happy paths hits BUG under -rt patch
Hi Leon, On Sat, 7 Nov 2009 20:01:59 +0100, Leon Woestenberg wrote: during testing the Linux PREEMP_RT work (step-by-step being merged with mainline) together with I2C functionality I hit the fact that the I2C subsystem uses yield() in some of the non-happy code paths (mostly during chip / address probing etc). The I2C subsystem itself doesn't; individual I2C bus drivers do. One of them is i2c-algo-bit, which is a helper module widely used by other I2C bus drivers. yield() is only used once in i2c-algo-bit, when the slave device we are talking to doesn't ack its address at first try (and adapter-retries is set to non-zero.) My (embedded) system was running a low-priority real-time work on the generic workqueue which tried to blink a LED using an I2C I/O multiplexer when I hit the BUG where this real-time task ran into the yield() of try_address(). One thing I do not understand: if yield() is a bug to RT kernels, then we would have to remove them all? But so far, yield() still exists in the kernel tree, and it serves a purpose. Are you going to ask all developers to remove all occurrences of yield() in their code? Doesn't sound terribly realistic. Grepping through the I2C subsystem code there where more yield() sprinkled in, without it being clear to me why they are there. The only occurrence I found is in driver i2c-bfin-twi, where it is used to wait until the bus is ready. The use of yield() make the busy-waiting less aggressive. Alternatives are sleeping (which I presume RT wouldn't like either) or pure busy-waiting (which doesn't sound terribly appealing, right?) Can those yield()s please be removed, and if they are needed for some reason (??) be replaced with something equivalent? I don't think yield() is ever needed. It is there when developers try to be fair to other running threads. I can't think of cases where anything will break when removing them, but system latency might suffer. Isn't it a little odd that in the name of RT, you're asking for people to remove a mechanism which was introduced to lower system latency? I think this all needs to be discussed at a higher level. Namely, RT people need to discuss how yield() should behave with regards to RT threads. Maybe it should simply become a no-op for these threads? (Disclaimer if it wasn't obvious yet: I don't know much about RT.) -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html