* [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY @ 2015-10-11 13:05 Gilles Chanteperdrix 2015-10-12 7:23 ` Jan Kiszka 0 siblings, 1 reply; 8+ messages in thread From: Gilles Chanteperdrix @ 2015-10-11 13:05 UTC (permalink / raw) To: Xenomai Hi, It seems commit fdb5d54d04b8c3b6b6a6ad7ac2b6248cf0b415e0 in the I-pipe kernels cause a warning when CONFIG_PREEMPT_VOLUNTARY and CONFIG_IPIPE_DEBUG_INTERNAL are enabled. The culprit is the call to __ipipe_root_p. The following patch avoids this warning: diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c index 0320453..a5e440d 100644 --- a/kernel/ipipe/core.c +++ b/kernel/ipipe/core.c @@ -1730,17 +1730,19 @@ int notrace __ipipe_check_percpu_access(void) if (raw_irqs_disabled_flags(flags)) goto out; + if (!IS_ENABLED(CONFIG_PREEMPT_COUNT)) + goto out; + /* * Last chance: hw interrupts were enabled on entry while * running over the root domain, but the root stage might be * currently stalled, in which case preemption would be * disabled, and no migration could occur. */ - if (this_domain == ipipe_root_domain) { - p = raw_cpu_ptr(&ipipe_percpu.root); - if (test_bit(IPIPE_STALL_FLAG, &p->status) || preempt_count()) + + p = raw_cpu_ptr(&ipipe_percpu.root); + if (test_bit(IPIPE_STALL_FLAG, &p->status) || preempt_count()) goto out; - } /* * Our caller may end up accessing the wrong per-cpu variable * instance due to CPU migration; tell it to complain about -- Gilles. https://click-hack.org ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY 2015-10-11 13:05 [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY Gilles Chanteperdrix @ 2015-10-12 7:23 ` Jan Kiszka 2015-10-12 8:10 ` Gilles Chanteperdrix 0 siblings, 1 reply; 8+ messages in thread From: Jan Kiszka @ 2015-10-12 7:23 UTC (permalink / raw) To: Gilles Chanteperdrix, Xenomai On 2015-10-11 15:05, Gilles Chanteperdrix wrote: > Hi, > > It seems commit fdb5d54d04b8c3b6b6a6ad7ac2b6248cf0b415e0 in the > I-pipe kernels cause a warning when CONFIG_PREEMPT_VOLUNTARY and > CONFIG_IPIPE_DEBUG_INTERNAL are enabled. The culprit is the call to > __ipipe_root_p. Compiler warning? Runtime warning? Which architecture? I assume that ftrace is on then, right? > > The following patch avoids this warning: How? > > diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c > index 0320453..a5e440d 100644 > --- a/kernel/ipipe/core.c > +++ b/kernel/ipipe/core.c > @@ -1730,17 +1730,19 @@ int notrace __ipipe_check_percpu_access(void) > if (raw_irqs_disabled_flags(flags)) > goto out; > > + if (!IS_ENABLED(CONFIG_PREEMPT_COUNT)) > + goto out; > + This approach looks a bit odd, but I simply may not understand it yet. We need to know what it actually aims at, i.e. which line causes the error you see. > /* > * Last chance: hw interrupts were enabled on entry while > * running over the root domain, but the root stage might be > * currently stalled, in which case preemption would be > * disabled, and no migration could occur. > */ > - if (this_domain == ipipe_root_domain) { This test is indeed redundant, but removing it should be a separate patch with a commit log referring to the test that is not visible in the context. > - p = raw_cpu_ptr(&ipipe_percpu.root); > - if (test_bit(IPIPE_STALL_FLAG, &p->status) || preempt_count()) > + > + p = raw_cpu_ptr(&ipipe_percpu.root); > + if (test_bit(IPIPE_STALL_FLAG, &p->status) || preempt_count()) > goto out; You probably want to adjust the indention of this line then. > - } > /* > * Our caller may end up accessing the wrong per-cpu variable > * instance due to CPU migration; tell it to complain about > > Jan -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY 2015-10-12 7:23 ` Jan Kiszka @ 2015-10-12 8:10 ` Gilles Chanteperdrix 2015-10-12 8:37 ` Jan Kiszka 0 siblings, 1 reply; 8+ messages in thread From: Gilles Chanteperdrix @ 2015-10-12 8:10 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On Mon, Oct 12, 2015 at 09:23:46AM +0200, Jan Kiszka wrote: > On 2015-10-11 15:05, Gilles Chanteperdrix wrote: > > Hi, > > > > It seems commit fdb5d54d04b8c3b6b6a6ad7ac2b6248cf0b415e0 in the > > I-pipe kernels cause a warning when CONFIG_PREEMPT_VOLUNTARY and > > CONFIG_IPIPE_DEBUG_INTERNAL are enabled. The culprit is the call to > > __ipipe_root_p. > > Compiler warning? Runtime warning? Which architecture? I assume that > ftrace is on then, right? Yes. Because preempt_disable()/preempt_enable*() is a nop without CONFIG_PREEMPT_COUNT, __ipipe_root_p ends up being called without any protection, and __ipipe_check_percpu_access() emits the warning. > > > > > The following patch avoids this warning: > > How? It disables the test for preemption (the call to preempt_count()) in __ipipe_check_percpu_access() if CONFIG_PREEMPT_COUNT is disabled, because if it is disabled, you can not be preempted anywhere, so calling __ipipe_root_p without protection is valid. Or maybe not? -- Gilles. https://click-hack.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY 2015-10-12 8:10 ` Gilles Chanteperdrix @ 2015-10-12 8:37 ` Jan Kiszka 2015-10-12 8:48 ` Gilles Chanteperdrix 0 siblings, 1 reply; 8+ messages in thread From: Jan Kiszka @ 2015-10-12 8:37 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2015-10-12 10:10, Gilles Chanteperdrix wrote: > On Mon, Oct 12, 2015 at 09:23:46AM +0200, Jan Kiszka wrote: >> On 2015-10-11 15:05, Gilles Chanteperdrix wrote: >>> Hi, >>> >>> It seems commit fdb5d54d04b8c3b6b6a6ad7ac2b6248cf0b415e0 in the >>> I-pipe kernels cause a warning when CONFIG_PREEMPT_VOLUNTARY and >>> CONFIG_IPIPE_DEBUG_INTERNAL are enabled. The culprit is the call to >>> __ipipe_root_p. >> >> Compiler warning? Runtime warning? Which architecture? I assume that >> ftrace is on then, right? > > Yes. Because preempt_disable()/preempt_enable*() is a nop without > CONFIG_PREEMPT_COUNT, __ipipe_root_p ends up being called without > any protection, and __ipipe_check_percpu_access() emits the warning. > Makes sense. >> >>> >>> The following patch avoids this warning: >> >> How? > > It disables the test for preemption (the call to preempt_count()) in > __ipipe_check_percpu_access() if CONFIG_PREEMPT_COUNT is disabled, > because if it is disabled, you can not be preempted anywhere, so > calling __ipipe_root_p without protection is valid. Or maybe not? Ah, ok. I was under the assumption preempt_count() would return a constant non-zero value under !CONFIG_PREEMPT. But on the other hand, that would be weird as well when triggering voluntary preemption then. Anyway, it's correct that preemption cannot happen then in the middle of an instruction, so we can skip the test. Please make the test depend on CONFIG_PREEMPT and leave a corresponding comment in the code. Thanks, Jan -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY 2015-10-12 8:37 ` Jan Kiszka @ 2015-10-12 8:48 ` Gilles Chanteperdrix 2015-10-12 8:58 ` Jan Kiszka 0 siblings, 1 reply; 8+ messages in thread From: Gilles Chanteperdrix @ 2015-10-12 8:48 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On Mon, Oct 12, 2015 at 10:37:50AM +0200, Jan Kiszka wrote: > On 2015-10-12 10:10, Gilles Chanteperdrix wrote: > > On Mon, Oct 12, 2015 at 09:23:46AM +0200, Jan Kiszka wrote: > >> On 2015-10-11 15:05, Gilles Chanteperdrix wrote: > >>> Hi, > >>> > >>> It seems commit fdb5d54d04b8c3b6b6a6ad7ac2b6248cf0b415e0 in the > >>> I-pipe kernels cause a warning when CONFIG_PREEMPT_VOLUNTARY and > >>> CONFIG_IPIPE_DEBUG_INTERNAL are enabled. The culprit is the call to > >>> __ipipe_root_p. > >> > >> Compiler warning? Runtime warning? Which architecture? I assume that > >> ftrace is on then, right? > > > > Yes. Because preempt_disable()/preempt_enable*() is a nop without > > CONFIG_PREEMPT_COUNT, __ipipe_root_p ends up being called without > > any protection, and __ipipe_check_percpu_access() emits the warning. > > > > Makes sense. > > >> > >>> > >>> The following patch avoids this warning: > >> > >> How? > > > > It disables the test for preemption (the call to preempt_count()) in > > __ipipe_check_percpu_access() if CONFIG_PREEMPT_COUNT is disabled, > > because if it is disabled, you can not be preempted anywhere, so > > calling __ipipe_root_p without protection is valid. Or maybe not? > > Ah, ok. I was under the assumption preempt_count() would return a > constant non-zero value under !CONFIG_PREEMPT. But on the other hand, > that would be weird as well when triggering voluntary preemption then. > > Anyway, it's correct that preemption cannot happen then in the middle of > an instruction, so we can skip the test. Please make the test depend on > CONFIG_PREEMPT and leave a corresponding comment in the code. maybe we could use preemptible()? it should test both the stall flag and preempt_count with CONFIG_PREEMPT_COUNT and returns 0 without CONFIG_PREEMPT_COUNT. -- Gilles. https://click-hack.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY 2015-10-12 8:48 ` Gilles Chanteperdrix @ 2015-10-12 8:58 ` Jan Kiszka 2015-10-12 17:40 ` [Xenomai] [PATCH 1/2] ipipe: simplify __ipipe_check_perccpu_access() Gilles Chanteperdrix 0 siblings, 1 reply; 8+ messages in thread From: Jan Kiszka @ 2015-10-12 8:58 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2015-10-12 10:48, Gilles Chanteperdrix wrote: > On Mon, Oct 12, 2015 at 10:37:50AM +0200, Jan Kiszka wrote: >> On 2015-10-12 10:10, Gilles Chanteperdrix wrote: >>> On Mon, Oct 12, 2015 at 09:23:46AM +0200, Jan Kiszka wrote: >>>> On 2015-10-11 15:05, Gilles Chanteperdrix wrote: >>>>> Hi, >>>>> >>>>> It seems commit fdb5d54d04b8c3b6b6a6ad7ac2b6248cf0b415e0 in the >>>>> I-pipe kernels cause a warning when CONFIG_PREEMPT_VOLUNTARY and >>>>> CONFIG_IPIPE_DEBUG_INTERNAL are enabled. The culprit is the call to >>>>> __ipipe_root_p. >>>> >>>> Compiler warning? Runtime warning? Which architecture? I assume that >>>> ftrace is on then, right? >>> >>> Yes. Because preempt_disable()/preempt_enable*() is a nop without >>> CONFIG_PREEMPT_COUNT, __ipipe_root_p ends up being called without >>> any protection, and __ipipe_check_percpu_access() emits the warning. >>> >> >> Makes sense. >> >>>> >>>>> >>>>> The following patch avoids this warning: >>>> >>>> How? >>> >>> It disables the test for preemption (the call to preempt_count()) in >>> __ipipe_check_percpu_access() if CONFIG_PREEMPT_COUNT is disabled, >>> because if it is disabled, you can not be preempted anywhere, so >>> calling __ipipe_root_p without protection is valid. Or maybe not? >> >> Ah, ok. I was under the assumption preempt_count() would return a >> constant non-zero value under !CONFIG_PREEMPT. But on the other hand, >> that would be weird as well when triggering voluntary preemption then. >> >> Anyway, it's correct that preemption cannot happen then in the middle of >> an instruction, so we can skip the test. Please make the test depend on >> CONFIG_PREEMPT and leave a corresponding comment in the code. > > maybe we could use preemptible()? > > it should test both the stall flag and preempt_count with > CONFIG_PREEMPT_COUNT and returns 0 without CONFIG_PREEMPT_COUNT. Indeed, even better. And self-documenting. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Xenomai] [PATCH 1/2] ipipe: simplify __ipipe_check_perccpu_access() 2015-10-12 8:58 ` Jan Kiszka @ 2015-10-12 17:40 ` Gilles Chanteperdrix 2015-10-12 17:40 ` [Xenomai] [PATCH 2/2] ipipe: fix __ipipe_check_percpu_access() Gilles Chanteperdrix 0 siblings, 1 reply; 8+ messages in thread From: Gilles Chanteperdrix @ 2015-10-12 17:40 UTC (permalink / raw) To: xenomai; +Cc: Gilles Chanteperdrix by removing a redundant test. --- kernel/ipipe/core.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c index 0320453..b02883d 100644 --- a/kernel/ipipe/core.c +++ b/kernel/ipipe/core.c @@ -1736,11 +1736,10 @@ int notrace __ipipe_check_percpu_access(void) * currently stalled, in which case preemption would be * disabled, and no migration could occur. */ - if (this_domain == ipipe_root_domain) { - p = raw_cpu_ptr(&ipipe_percpu.root); - if (test_bit(IPIPE_STALL_FLAG, &p->status) || preempt_count()) - goto out; - } + + p = raw_cpu_ptr(&ipipe_percpu.root); + if (test_bit(IPIPE_STALL_FLAG, &p->status) || preempt_count()) + goto out; /* * Our caller may end up accessing the wrong per-cpu variable * instance due to CPU migration; tell it to complain about -- 1.8.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Xenomai] [PATCH 2/2] ipipe: fix __ipipe_check_percpu_access() 2015-10-12 17:40 ` [Xenomai] [PATCH 1/2] ipipe: simplify __ipipe_check_perccpu_access() Gilles Chanteperdrix @ 2015-10-12 17:40 ` Gilles Chanteperdrix 0 siblings, 0 replies; 8+ messages in thread From: Gilles Chanteperdrix @ 2015-10-12 17:40 UTC (permalink / raw) To: xenomai; +Cc: Gilles Chanteperdrix Use preemptible() instead of testing the preempt_count and the stall flags, since using preemptible() also works when CONFIG_PREEMPT_COUNT is not defined. --- kernel/ipipe/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c index b02883d..d7c8934 100644 --- a/kernel/ipipe/core.c +++ b/kernel/ipipe/core.c @@ -1738,7 +1738,7 @@ int notrace __ipipe_check_percpu_access(void) */ p = raw_cpu_ptr(&ipipe_percpu.root); - if (test_bit(IPIPE_STALL_FLAG, &p->status) || preempt_count()) + if (!preemptible()) goto out; /* * Our caller may end up accessing the wrong per-cpu variable -- 1.8.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-10-12 17:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-11 13:05 [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY Gilles Chanteperdrix 2015-10-12 7:23 ` Jan Kiszka 2015-10-12 8:10 ` Gilles Chanteperdrix 2015-10-12 8:37 ` Jan Kiszka 2015-10-12 8:48 ` Gilles Chanteperdrix 2015-10-12 8:58 ` Jan Kiszka 2015-10-12 17:40 ` [Xenomai] [PATCH 1/2] ipipe: simplify __ipipe_check_perccpu_access() Gilles Chanteperdrix 2015-10-12 17:40 ` [Xenomai] [PATCH 2/2] ipipe: fix __ipipe_check_percpu_access() Gilles Chanteperdrix
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.