[Adeos-main] irq masking in ipipe_restore_pipeline_head

[Adeos-main] irq masking in ipipe_restore_pipeline_head

[Jan Kiszka]

Hi Philippe,

don't we need this in ipipe_restore_pipeline_head to play safe (includes
some UP optimization as well):

diff --git a/include/linux/ipipe.h b/include/linux/ipipe.h
index c458883..bf1f94d 100644
--- a/include/linux/ipipe.h
+++ b/include/linux/ipipe.h
@@ -518,9 +518,13 @@ void __ipipe_restore_pipeline_head(unsigned long x);
 
 static inline void ipipe_restore_pipeline_head(unsigned long x)
 {
-	local_irq_disable_hw();
+	unsigned long flags;
+
+	local_irq_save_hw_smp(flags);
 	if ((x ^ test_bit(IPIPE_STALL_FLAG, &ipipe_head_cpudom_var(status))) & 1)
 		__ipipe_restore_pipeline_head(x);
+	else
+		local_irq_restore_hw_smp(flags);
 }
 
 #define ipipe_unstall_pipeline() \
diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
index e2503bd..60a6433 100644
--- a/kernel/ipipe/core.c
+++ b/kernel/ipipe/core.c
@@ -422,6 +422,9 @@ void __ipipe_restore_pipeline_head(unsigned long x) /* hw interrupt off */
 	struct ipipe_percpu_domain_data *p = ipipe_head_cpudom_ptr();
 	struct ipipe_domain *head_domain;
 
+#ifndef CONFIG_SMP
+	local_irq_disable_hw();
+#endif
 	if (x) {
 #ifdef CONFIG_DEBUG_KERNEL
 		static int warned;


Untested, just thoughts ATM.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] irq masking in ipipe_restore_pipeline_head

[Philippe Gerum]

On Fri, 2010-06-18 at 13:26 +0200, Jan Kiszka wrote:
> Hi Philippe,
> 
> don't we need this in ipipe_restore_pipeline_head to play safe

I can't see what this is supposed to fix. Any hint?

>  (includes
> some UP optimization as well):
> 
> diff --git a/include/linux/ipipe.h b/include/linux/ipipe.h
> index c458883..bf1f94d 100644
> --- a/include/linux/ipipe.h
> +++ b/include/linux/ipipe.h
> @@ -518,9 +518,13 @@ void __ipipe_restore_pipeline_head(unsigned long x);
>  
>  static inline void ipipe_restore_pipeline_head(unsigned long x)
>  {
> -	local_irq_disable_hw();
> +	unsigned long flags;
> +
> +	local_irq_save_hw_smp(flags);
>  	if ((x ^ test_bit(IPIPE_STALL_FLAG, &ipipe_head_cpudom_var(status))) & 1)
>  		__ipipe_restore_pipeline_head(x);
> +	else
> +		local_irq_restore_hw_smp(flags);
>  }
>  
>  #define ipipe_unstall_pipeline() \
> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> index e2503bd..60a6433 100644
> --- a/kernel/ipipe/core.c
> +++ b/kernel/ipipe/core.c
> @@ -422,6 +422,9 @@ void __ipipe_restore_pipeline_head(unsigned long x) /* hw interrupt off */
>  	struct ipipe_percpu_domain_data *p = ipipe_head_cpudom_ptr();
>  	struct ipipe_domain *head_domain;
>  
> +#ifndef CONFIG_SMP
> +	local_irq_disable_hw();
> +#endif
>  	if (x) {
>  #ifdef CONFIG_DEBUG_KERNEL
>  		static int warned;
> 
> 
> Untested, just thoughts ATM.
> 
> Jan
> 


-- 
Philippe.

Re: [Adeos-main] irq masking in ipipe_restore_pipeline_head

[Jan Kiszka]

Philippe Gerum wrote:
> On Fri, 2010-06-18 at 13:26 +0200, Jan Kiszka wrote:
>> Hi Philippe,
>>
>> don't we need this in ipipe_restore_pipeline_head to play safe
> 
> I can't see what this is supposed to fix. Any hint?

You enter ipipe_restore_pipeline_head with hard IRQs enabled and you
don't take the slow path as the domain is not stalled and the passed
flags do not change this. Then you leave with hard IRQs disabled. Corner
case, but I think it's not per se impossible.

Jan

> 
>>  (includes
>> some UP optimization as well):
>>
>> diff --git a/include/linux/ipipe.h b/include/linux/ipipe.h
>> index c458883..bf1f94d 100644
>> --- a/include/linux/ipipe.h
>> +++ b/include/linux/ipipe.h
>> @@ -518,9 +518,13 @@ void __ipipe_restore_pipeline_head(unsigned long x);
>>  
>>  static inline void ipipe_restore_pipeline_head(unsigned long x)
>>  {
>> -	local_irq_disable_hw();
>> +	unsigned long flags;
>> +
>> +	local_irq_save_hw_smp(flags);
>>  	if ((x ^ test_bit(IPIPE_STALL_FLAG, &ipipe_head_cpudom_var(status))) & 1)
>>  		__ipipe_restore_pipeline_head(x);
>> +	else
>> +		local_irq_restore_hw_smp(flags);
>>  }
>>  
>>  #define ipipe_unstall_pipeline() \
>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>> index e2503bd..60a6433 100644
>> --- a/kernel/ipipe/core.c
>> +++ b/kernel/ipipe/core.c
>> @@ -422,6 +422,9 @@ void __ipipe_restore_pipeline_head(unsigned long x) /* hw interrupt off */
>>  	struct ipipe_percpu_domain_data *p = ipipe_head_cpudom_ptr();
>>  	struct ipipe_domain *head_domain;
>>  
>> +#ifndef CONFIG_SMP
>> +	local_irq_disable_hw();
>> +#endif
>>  	if (x) {
>>  #ifdef CONFIG_DEBUG_KERNEL
>>  		static int warned;
>>
>>
>> Untested, just thoughts ATM.
>>
>> Jan
>>
> 
> 

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] irq masking in ipipe_restore_pipeline_head

[Philippe Gerum]

On Fri, 2010-06-18 at 15:46 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Fri, 2010-06-18 at 13:26 +0200, Jan Kiszka wrote:
> >> Hi Philippe,
> >>
> >> don't we need this in ipipe_restore_pipeline_head to play safe
> > 
> > I can't see what this is supposed to fix. Any hint?
> 
> You enter ipipe_restore_pipeline_head with hard IRQs enabled and you
> don't take the slow path as the domain is not stalled and the passed
> flags do not change this. Then you leave with hard IRQs disabled. Corner
> case, but I think it's not per se impossible.

In fact, the semantics of raw_local_irq_restore() are not that specific,
looking at various irqflags.h implementations.
Architectures are allowed to apply a local optimization for that
routine. They may only re-enable if the flags arg passed says irqs were
on previously to setting them off (e.g. blackfin), or even test a
private system variable to know the current hw state and act upon this,
ignoring the flags argument totally for this purpose (e.g. ppc64). There
are likely others, since flipping the irq mask is costly for some archs,
so optimizing makes sense for them.

In short, __ipipe_restore_pipeline_head() does not guarantee more than
the kernel does with raw_local_irq_restore(), meaning that it is only
required to switch irqs on if they were set off by the matching
__ipipe_test_and_stall_pipeline_head(), but nothing more.

However, it would make sense to detect such misuse of the interface and
whine loudly whenever we enter __ipipe_restore_pipeline_head() with irqs
on.

> 
> Jan
> 
> > 
> >>  (includes
> >> some UP optimization as well):
> >>
> >> diff --git a/include/linux/ipipe.h b/include/linux/ipipe.h
> >> index c458883..bf1f94d 100644
> >> --- a/include/linux/ipipe.h
> >> +++ b/include/linux/ipipe.h
> >> @@ -518,9 +518,13 @@ void __ipipe_restore_pipeline_head(unsigned long x);
> >>  
> >>  static inline void ipipe_restore_pipeline_head(unsigned long x)
> >>  {
> >> -	local_irq_disable_hw();
> >> +	unsigned long flags;
> >> +
> >> +	local_irq_save_hw_smp(flags);
> >>  	if ((x ^ test_bit(IPIPE_STALL_FLAG, &ipipe_head_cpudom_var(status))) & 1)
> >>  		__ipipe_restore_pipeline_head(x);
> >> +	else
> >> +		local_irq_restore_hw_smp(flags);
> >>  }
> >>  
> >>  #define ipipe_unstall_pipeline() \
> >> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> >> index e2503bd..60a6433 100644
> >> --- a/kernel/ipipe/core.c
> >> +++ b/kernel/ipipe/core.c
> >> @@ -422,6 +422,9 @@ void __ipipe_restore_pipeline_head(unsigned long x) /* hw interrupt off */
> >>  	struct ipipe_percpu_domain_data *p = ipipe_head_cpudom_ptr();
> >>  	struct ipipe_domain *head_domain;
> >>  
> >> +#ifndef CONFIG_SMP
> >> +	local_irq_disable_hw();
> >> +#endif
> >>  	if (x) {
> >>  #ifdef CONFIG_DEBUG_KERNEL
> >>  		static int warned;
> >>
> >>
> >> Untested, just thoughts ATM.
> >>
> >> Jan
> >>
> > 
> > 
> 


-- 
Philippe.

Re: [Adeos-main] irq masking in ipipe_restore_pipeline_head

[Jan Kiszka]

Philippe Gerum wrote:
> On Fri, 2010-06-18 at 15:46 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Fri, 2010-06-18 at 13:26 +0200, Jan Kiszka wrote:
>>>> Hi Philippe,
>>>>
>>>> don't we need this in ipipe_restore_pipeline_head to play safe
>>> I can't see what this is supposed to fix. Any hint?
>> You enter ipipe_restore_pipeline_head with hard IRQs enabled and you
>> don't take the slow path as the domain is not stalled and the passed
>> flags do not change this. Then you leave with hard IRQs disabled. Corner
>> case, but I think it's not per se impossible.
> 
> In fact, the semantics of raw_local_irq_restore() are not that specific,
> looking at various irqflags.h implementations.
> Architectures are allowed to apply a local optimization for that
> routine. They may only re-enable if the flags arg passed says irqs were
> on previously to setting them off (e.g. blackfin), or even test a
> private system variable to know the current hw state and act upon this,
> ignoring the flags argument totally for this purpose (e.g. ppc64). There
> are likely others, since flipping the irq mask is costly for some archs,
> so optimizing makes sense for them.
> 
> In short, __ipipe_restore_pipeline_head() does not guarantee more than
> the kernel does with raw_local_irq_restore(), meaning that it is only
> required to switch irqs on if they were set off by the matching
> __ipipe_test_and_stall_pipeline_head(), but nothing more.
> 
> However, it would make sense to detect such misuse of the interface and
> whine loudly whenever we enter __ipipe_restore_pipeline_head() with irqs
> on.

You mean something like

if (WARN_ON_ONCE(!irqs_disabled_hw()))
	local_irq_disable_hw();

?

(Which leaves the if path even under !CONFIG_BUG. Maybe we want a bit
more optimization.)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux