Re: [Xenomai] ftrace dispatch function broken on 2.6.5

[Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>]

On Sat, Jun 18, 2016 at 03:21:40PM +0200, Philippe Gerum wrote:
> On 06/18/2016 03:08 PM, Gilles Chanteperdrix wrote:
> > Hi Philippe,
> > 
> > it seems since I-pipe commit
> > b115c4094d734e19fa7a96be1bf3958b3d244b8b on the ipipe-3.18 branch:
> >     Revert "ipipe: Register function tracer for direct and exclusive invocation"
> >     
> >     This reverts commit e00888b4aae45d9b84698a62079dde14c9be5fd3.
> >     
> >     We now have an I-pipe-compatible dispatching function for ftrace.
> > 
> > The ftrace dispatching function causes the following warning at
> > boot on x86_32 with all warnings/debugs enabled:
> > [    4.730812] I-pipe: head domain Xenomai registered.
> > [    4.737967] I-pipe: Detected illicit call from head domain 'Xenomai'
> > [    4.737967]         into a regular Linux service
> > 
> > Because it calls preempt_disable(), which is not safe to be called
> > form root domain, when runnning over 2.6.x on an architecture such
> > as x86_32 which does not have IPIPE_HAVE_SAFE_THREAD_INFO.
> > 
> > Should we make the ftrace dispatching function really I-pipe
> > compatible by calling ipipe_preempt_disable() in that case instead?
> > or should we make the patch revert conditional to !IPIPE_LEGACY or
> > IPIPE_HAVE_SAFE_THREAD_INFO (but that would make only the I-pipe
> > tracer work in that case).
> > 
> 
> I would go for the change which has the lesser impact on the mainline
> code; that would be option #1.

Ok. Something else. Commit fdb5d54d04b8c3b6b6a6ad7ac2b6248cf0b415e0:
    ipipe: Avoid rescheduling from __ftrace_ops_list_func in illegal contexts
    
    The ftrace callback dispatcher may be called with hard irqs disabled or
    even over the head domain. We cannot allow any rescheduling in that
    case, but also do not have: only non-urgent events are expected to be
    kicked-off from ftrace callbacks, and those will at latest be handled on
    the next Linux timer tick.

Added the following code to the ftrace dispatch function:
+#ifdef CONFIG_IPIPE
+       if (hard_irqs_disabled() || !__ipipe_root_p)
+               /*
+                * Nothing urgent to schedule here. At latest the timer tick
+                * will pick up whatever the tracing functions kicked off.
+                */
+               preempt_enable_no_resched_notrace();
+       else
+#endif
+               preempt_enable_notrace();

Should not this go to the generic definition of preempt_enable() ? I
mean if in the !LEGACY case, it is now legal to call
preempt_enable() over non root contexts or with hard irqs off, so,
does it really make sense to fix all the preempt_enable() spots one
by one?

-- 
					    Gilles.
https://click-hack.org