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.
_______________________________________________
Adeos-main mailing list
[email protected]
https://mail.gna.org/listinfo/adeos-main
