Re: [Xenomai-core] Potential problem with rt_eepro100

[Philippe Gerum <rpm@xenomai.org>]

On Sat, 2010-11-06 at 21:37 +0100, Gilles Chanteperdrix wrote:
> Anders Blomdell wrote:
> > Gilles Chanteperdrix wrote:
> >> Anders Blomdell wrote:
> >>> Gilles Chanteperdrix wrote:
> >>>> Jan Kiszka wrote:
> >>>>> Am 05.11.2010 00:24, Gilles Chanteperdrix wrote:
> >>>>>> Jan Kiszka wrote:
> >>>>>>> Am 04.11.2010 23:06, Gilles Chanteperdrix wrote:
> >>>>>>>> Jan Kiszka wrote:
> >>>>>>>>>>> At first sight, here you are more breaking things than cleaning them.
> >>>>>>>>>> Still, it has the SMP record for my test program, still runs with ftrace 
> >>>>>>>>>> on (after 2 hours, where it previously failed after maximum 23 minutes).
> >>>>>>>>> My version was indeed still buggy, I'm reworking it ATM.
> >>>>>>>>>
> >>>>>>>>>> If I get the gist of Jan's changes, they are (using the IPI to transfer 
> >>>>>>>>>> one bit of information: your cpu needs to reschedule):
> >>>>>>>>>>
> >>>>>>>>>> xnsched_set_resched:
> >>>>>>>>>> -      setbits((__sched__)->status, XNRESCHED);
> >>>>>>>>>>
> >>>>>>>>>> xnpod_schedule_handler:
> >>>>>>>>>> +	xnsched_set_resched(sched);
> >>>>>>>>>> 	
> >>>>>>>>>> If you (we?) decide to keep the debug checks, under what circumstances 
> >>>>>>>>>> would the current check trigger (in laymans language, that I'll be able 
> >>>>>>>>>> to understand)?
> >>>>>>>>> That's actually what /me is wondering as well. I do not see yet how you
> >>>>>>>>> can reliably detect a missed reschedule reliably (that was the purpose
> >>>>>>>>> of the debug check) given the racy nature between signaling resched and
> >>>>>>>>> processing the resched hints.
> >>>>>>>> The purpose of the debugging change is to detect a change of the
> >>>>>>>> scheduler state which was not followed by setting the XNRESCHED bit.
> >>>>>>> But that is nucleus business, nothing skins can screw up (as long as
> >>>>>>> they do not misuse APIs).
> >>>>>> Yes, but it happens that we modify the nucleus from time to time.
> >>>>>>
> >>>>>>>> Getting it to work is relatively simple: we add a "scheduler change set
> >>>>>>>> remotely" bit to the sched structure which is NOT in the status bit, set
> >>>>>>>> this bit when changing a remote sched (under nklock). In the debug check
> >>>>>>>> code, if the scheduler state changed, and the XNRESCHED bit is not set,
> >>>>>>>> only consider this a but if this new bit is not set. All this is
> >>>>>>>> compiled out if the debug is not enabled.
> >>>>>>> I still see no benefit in this check. Where to you want to place the bit
> >>>>>>> set? Aren't that just the same locations where
> >>>>>>> xnsched_set_[self_]resched already is today?
> >>>>>> Well no, that would be another bit in the sched structure which would
> >>>>>> allow us to manipulate the status bits from the local cpu. That
> >>>>>> supplementary bit would only be changed from a distant CPU, and serve to
> >>>>>> detect the race which causes the false positive. The resched bits are
> >>>>>> set on the local cpu to get xnpod_schedule to trigger a rescheduling on
> >>>>>> the distance cpu. That bit would be set on the remote cpu's sched. Only
> >>>>>> when debugging is enabled.
> >>>>>>
> >>>>>>> But maybe you can provide some motivating bug scenarios, real ones of
> >>>>>>> the past or realistic ones of the future.
> >>>>>> Of course. The bug is anything which changes the scheduler state but
> >>>>>> does not set the XNRESCHED bit. This happened when we started the SMP
> >>>>>> port. New scheduling policies would be good candidates for a revival of
> >>>>>> this bug.
> >>>>>>
> >>>>> You don't gain any worthwhile check if you cannot make the
> >>>>> instrumentation required for a stable detection simpler than the proper
> >>>>> problem solution itself. And this is what I'm still skeptical of.
> >>>> The solution is simple, but finding the problem without the 
> >>>> instrumentation is way harder than with the instrumentation, so the 
> >>>> instrumentation is worth something.
> >>>>
> >>>> Reproducing the false positive is surprisingly easy with a simple
> >>>> dual-cpu semaphore ping-pong test. So, here is the (tested) patch, 
> >>>> using a ridiculous long variable name to illustrate what I was 
> >>>> thinking about:
> >>>>
> >>>> diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
> >>>> index 8888cf4..454b8e8 100644
> >>>> --- a/include/nucleus/sched.h
> >>>> +++ b/include/nucleus/sched.h
> >>>> @@ -108,6 +108,9 @@ typedef struct xnsched {
> >>>>         struct xnthread *gktarget;
> >>>>  #endif
> >>>>
> >>>> +#ifdef CONFIG_XENO_OPT_DEBUG_NUCLEUS
> >>>> +       int debug_resched_from_remote;
> >>>> +#endif
> >>>>  } xnsched_t;
> >>>>
> >>>>  union xnsched_policy_param;
> >>>> @@ -185,6 +188,8 @@ static inline int xnsched_resched_p(struct xnsched *sched)
> >>>>    xnsched_t *current_sched = xnpod_current_sched();                    \
> >>>>    __setbits(current_sched->status, XNRESCHED);                         \
> >>>>    if (current_sched != (__sched__))    {                               \
> >>>> +         if (XENO_DEBUG(NUCLEUS))                                      \
> >>>> +                 __sched__->debug_resched_from_remote = 1;             \
> >>>>        xnarch_cpu_set(xnsched_cpu(__sched__), current_sched->resched);  \
> >>>>    }                                                                    \
> >>>>  } while (0)
> >>>> diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
> >>>> index 4cb707a..50b0f49 100644
> >>>> --- a/ksrc/nucleus/pod.c
> >>>> +++ b/ksrc/nucleus/pod.c
> >>>> @@ -2177,6 +2177,10 @@ static inline int __xnpod_test_resched(struct xnsched *sched)
> >>>>                 xnarch_cpus_clear(sched->resched);
> >>>>         }
> >>>>  #endif
> >>>> +       if (XENO_DEBUG(NUCLEUS) && sched->debug_resched_from_remote) {
> >>>> +               sched->debug_resched_from_remote = 0;
> >>>> +               resched = 1;
> >>>> +       }
> >>>>         clrbits(sched->status, XNRESCHED);
> >>>>         return resched;
> >>>>  }
> >>>>
> >>>>
> >>>> I am still uncertain.
> >>> Will only work if all is done under nklock, otherwise two almost 
> >>> simultaneous xnsched_resched_p from different cpus, might lead to one of 
> >>> the ipi wakeups sees the 0 written due to handling the first ipi interrupt.
> >> This is a patch artifact, the function modified are xnsched_set_resched
> >> and xnpod_test_resched, and both are run with the nklock locked.
> >>
> > 
> > Isn't this a possible scenario?
> > 
> > CPU A			CPU B				CPU C
> > take nklock
> > remote = 1
> > send ipi #1
> > release nklock					
> > 			take nklock			handle ipi
> > 			remote = 1			ack ipi #1
> > 			send ipi #2
> > 			release nklock
> > 							take nklock
> > 							if remote (==1)
> > 							  remote = 0
> > 							  reseched = 1
> > 							relese nklock
> > 							handle ipi
> > 							ack ipi #2
> > 							take nklock
> > 							if remote (==0)
> > 							  OOPS!
> 
> No problem here, since handling the first IPI has taken into account the
> two scheduler state changes. So, no OOPS. The second IPI is spurious.
> 
> Anyway, after some thoughts, I think we are going to try and make the
> current situation work instead of going back to the old way.
> 
> You can find the patch which attempts to do so here:
> http://sisyphus.hd.free.fr/~gilles/sched_status.txt

Ack. At last, this addresses the real issues without asking for
regression funkiness: fix the lack of barrier before testing XNSCHED in
the xnpod_schedule pre-test, and stop sched->status trashing due to
XNINIRQ/XNHTICK/XNRPICK ops done un-synced on nklock.

In short, this patch looks like moving the local-only flags where they
belong, i.e. anywhere you want but *outside* of the status with remotely
accessed bits. XNRPICK seems to be handled differently, but it makes
sense to group it with other RPI data as you did, so fine with me.

> 
> It even avoids the second IPI in this case. Experimental and only
> lightly tested for now.
> 

-- 
Philippe.