[Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY

[Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY

[Gilles Chanteperdrix]

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

Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY

[Jan Kiszka]

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

Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY

[Gilles Chanteperdrix]

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

Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY

[Jan Kiszka]

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

Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY

[Gilles Chanteperdrix]

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

Re: [Xenomai] Issue with CONFIG_PREEMPT_VOLUNTARY

[Jan Kiszka]

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

[Xenomai] [PATCH 1/2] ipipe: simplify __ipipe_check_perccpu_access()

[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

[Xenomai] [PATCH 2/2] ipipe: fix __ipipe_check_percpu_access()

[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