subject:"\[Xenomai\-core\] Potential problem with rt

Am 07.11.2010 09:31, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 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

 Check the kernel, we actually need it on both sides. Wherever the final
 barriers will be, we should leave a comment behind why they are there.
 Could be picked up from kernel/smp.c.
 
 We have it on both sides: the non-local flags are modified while holding
 the nklock. Unlocking the nklock implies a barrier.

I think this does not work if nklock is used nested, ie. hold across
xnsched_set_resched and the next xnpod_schedule.

 

 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.

 I just hope we finally converge over a solution. Looks like all
 possibilities have been explored now. A few more comments on this one:

 It probably makes sense to group the status bits accordingly (both their
 values and definitions) and briefly document on which status field they
 are supposed to be applied.
 
 Ok, but I wanted them to not use the same values, so that we can use the
 sched-status | sched-lstatus trick in xnpod_schedule. Something is
 lacking too: we probably need to use sched-status | sched-lstatus for
 display in /proc.

Nothing speaks against clustering the flag values so that they can be
OR'ed. They should just be grouped according to their target field.

 

 I do not understand the split logic - or some bits are simply not yet
 migrated. XNHDEFER, XNSWLOCK, XNKCOUT are all local-only as well, no?
 Then better put them in the _local_ status field, that's more consistent
 (and would help if we once wanted to optimize their cache line usage).
 
 Maybe the naming is not good the. -status is everything which is
 modified under nklock, -lstatus is for XNINIRQ and XNHTICK which are
 modified without holding the nklock.

And this is not a good clustering IMHO as it makes no difference for a
local-only flag if it is protected by nklock or not (as long as IRQs are
off, of course). What makes a difference it the CPU using the related
field. If we group according to local and remote usage, less cache line
bouncing can be achieved (of both fields are also cache line aligned,
but that is a second step that only helps if the first is made).

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-07 Thread Gilles Chanteperdrix

Jan Kiszka wrote:
 Am 07.11.2010 09:31, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 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
 Check the kernel, we actually need it on both sides. Wherever the final
 barriers will be, we should leave a comment behind why they are there.
 Could be picked up from kernel/smp.c.
 We have it on both sides: the non-local flags are modified while holding
 the nklock. Unlocking the nklock implies a barrier.
 
 I think this does not work if nklock is used nested, ie. hold across
 xnsched_set_resched and the next xnpod_schedule.

Ok. There is a race here, even without nesting, we need a barrier before
sending the IPI.

 I do not understand the split logic - or some bits are simply not yet
 migrated. XNHDEFER, XNSWLOCK, XNKCOUT are all local-only as well, no?
 Then better put them in the _local_ status field, that's more consistent
 (and would help if we once wanted to optimize their cache line usage).
 Maybe the naming is not good the. -status is everything which is
 modified under nklock, -lstatus is for XNINIRQ and XNHTICK which are
 modified without holding the nklock.
 
 And this is not a good clustering IMHO as it makes no difference for a
 local-only flag if it is protected by nklock or not (as long as IRQs are
 off, of course). What makes a difference it the CPU using the related
 field. If we group according to local and remote usage, less cache line
 bouncing can be achieved (of both fields are also cache line aligned,
 but that is a second step that only helps if the first is made).

The two status are in the same cache line.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-07 Thread Philippe Gerum

On Sun, 2010-11-07 at 09:31 +0100, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
  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
  
  Check the kernel, we actually need it on both sides. Wherever the final
  barriers will be, we should leave a comment behind why they are there.
  Could be picked up from kernel/smp.c.
 
 We have it on both sides: the non-local flags are modified while holding
 the nklock. Unlocking the nklock implies a barrier.

I think we may have an issue with this kind of construct:

xnlock_get_irq*(nklock)
xnpod_resume/suspend/whatever_thread()
xnlock_get_irq*(nklock)
...
xnlock_put_irq*(nklock)
xnpod_schedule()
xnlock_get_irq*(nklock)
send_ipi
= xnpod_schedule_handler on dest CPU
xnlock_put_irq*(nklock)
xnlock_put_irq*(nklock)

The issue would be triggered by the use of recursive locking. In that
case, the source CPU would only sync its cache when the lock is actually
dropped by the outer xnlock_put_irq* call and the inner
xnlock_get/put_irq* would not act as barriers, so the remote
rescheduling handler won't always see the XNSCHED update done remotely,
and may lead to a no-op. So we need a barrier before sending the IPI in
__xnpod_test_resched().

This could not happen if all schedule state changes where clearly
isolated from rescheduling calls in different critical sections, but
it's sometimes not an option not to group them for consistency reasons.

 
  
  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.
  
  I just hope we finally converge over a solution. Looks like all
  possibilities have been explored now. A few more comments on this one:
  
  It probably makes sense to group the status bits accordingly (both their
  values and definitions) and briefly document on which status field they
  are supposed to be applied.
 
 Ok, but I wanted them to not use the same values, so that we can use the
 sched-status | sched-lstatus trick in xnpod_schedule. Something is
 lacking too: we probably need to use sched-status | sched-lstatus for
 display in /proc.
 
  
  I do not understand the split logic - or some bits are simply not yet
  migrated. XNHDEFER, XNSWLOCK, XNKCOUT are all local-only as well, no?
  Then better put them in the _local_ status field, that's more consistent
  (and would help if we once wanted to optimize their cache line usage).
 
 Maybe the naming is not good the. -status is everything which is
 modified under nklock, -lstatus is for XNINIRQ and XNHTICK which are
 modified without holding the nklock.
 
  
  The naming is unfortunate: status vs. lstatus. This is asking for
  confusion and typos. They must be better distinguishable, e.g.
  local_status. Or we need accessors that have debug checks built in,
  catching wrong bits for their target fields.
 
 I agree.
 
  
  Good catch of the RPI breakage, Gilles!
 

-- 
Philippe.



___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 07.11.2010 10:57, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 07.11.2010 09:31, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 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
 Check the kernel, we actually need it on both sides. Wherever the final
 barriers will be, we should leave a comment behind why they are there.
 Could be picked up from kernel/smp.c.
 We have it on both sides: the non-local flags are modified while holding
 the nklock. Unlocking the nklock implies a barrier.

 I think this does not work if nklock is used nested, ie. hold across
 xnsched_set_resched and the next xnpod_schedule.
 
 Ok. There is a race here, even without nesting, we need a barrier before
 sending the IPI.
 
 I do not understand the split logic - or some bits are simply not yet
 migrated. XNHDEFER, XNSWLOCK, XNKCOUT are all local-only as well, no?
 Then better put them in the _local_ status field, that's more consistent
 (and would help if we once wanted to optimize their cache line usage).
 Maybe the naming is not good the. -status is everything which is
 modified under nklock, -lstatus is for XNINIRQ and XNHTICK which are
 modified without holding the nklock.

 And this is not a good clustering IMHO as it makes no difference for a
 local-only flag if it is protected by nklock or not (as long as IRQs are
 off, of course). What makes a difference it the CPU using the related
 field. If we group according to local and remote usage, less cache line
 bouncing can be achieved (of both fields are also cache line aligned,
 but that is a second step that only helps if the first is made).
 
 The two status are in the same cache line.
 

Which can (and likely should) be changed - if the fields are logically
separated first.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 07.11.2010 11:03, Philippe Gerum wrote:
 On Sun, 2010-11-07 at 09:31 +0100, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 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

 Check the kernel, we actually need it on both sides. Wherever the final
 barriers will be, we should leave a comment behind why they are there.
 Could be picked up from kernel/smp.c.

 We have it on both sides: the non-local flags are modified while holding
 the nklock. Unlocking the nklock implies a barrier.
 
 I think we may have an issue with this kind of construct:
 
 xnlock_get_irq*(nklock)
   xnpod_resume/suspend/whatever_thread()
   xnlock_get_irq*(nklock)
   ...
   xnlock_put_irq*(nklock)
   xnpod_schedule()
   xnlock_get_irq*(nklock)
   send_ipi
   = xnpod_schedule_handler on dest CPU
   xnlock_put_irq*(nklock)
 xnlock_put_irq*(nklock)
 
 The issue would be triggered by the use of recursive locking. In that
 case, the source CPU would only sync its cache when the lock is actually
 dropped by the outer xnlock_put_irq* call and the inner
 xnlock_get/put_irq* would not act as barriers, so the remote
 rescheduling handler won't always see the XNSCHED update done remotely,
 and may lead to a no-op. So we need a barrier before sending the IPI in
 __xnpod_test_resched().

That's what I said.

And we need it on the reader side as an rmb().

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-07 Thread Gilles Chanteperdrix

Jan Kiszka wrote:
 Am 07.11.2010 11:03, Philippe Gerum wrote:
 On Sun, 2010-11-07 at 09:31 +0100, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 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
 Check the kernel, we actually need it on both sides. Wherever the final
 barriers will be, we should leave a comment behind why they are there.
 Could be picked up from kernel/smp.c.
 We have it on both sides: the non-local flags are modified while holding
 the nklock. Unlocking the nklock implies a barrier.
 I think we may have an issue with this kind of construct:

 xnlock_get_irq*(nklock)
  xnpod_resume/suspend/whatever_thread()
  xnlock_get_irq*(nklock)
  ...
  xnlock_put_irq*(nklock)
  xnpod_schedule()
  xnlock_get_irq*(nklock)
  send_ipi
  = xnpod_schedule_handler on dest CPU
  xnlock_put_irq*(nklock)
 xnlock_put_irq*(nklock)

 The issue would be triggered by the use of recursive locking. In that
 case, the source CPU would only sync its cache when the lock is actually
 dropped by the outer xnlock_put_irq* call and the inner
 xnlock_get/put_irq* would not act as barriers, so the remote
 rescheduling handler won't always see the XNSCHED update done remotely,
 and may lead to a no-op. So we need a barrier before sending the IPI in
 __xnpod_test_resched().
 
 That's what I said.
 
 And we need it on the reader side as an rmb().

This one we have, in xnpod_schedule_handler.


-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 07.11.2010 11:12, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 07.11.2010 11:03, Philippe Gerum wrote:
 On Sun, 2010-11-07 at 09:31 +0100, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 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
 Check the kernel, we actually need it on both sides. Wherever the final
 barriers will be, we should leave a comment behind why they are there.
 Could be picked up from kernel/smp.c.
 We have it on both sides: the non-local flags are modified while holding
 the nklock. Unlocking the nklock implies a barrier.
 I think we may have an issue with this kind of construct:

 xnlock_get_irq*(nklock)
 xnpod_resume/suspend/whatever_thread()
 xnlock_get_irq*(nklock)
 ...
 xnlock_put_irq*(nklock)
 xnpod_schedule()
 xnlock_get_irq*(nklock)
 send_ipi
 = xnpod_schedule_handler on dest CPU
 xnlock_put_irq*(nklock)
 xnlock_put_irq*(nklock)

 The issue would be triggered by the use of recursive locking. In that
 case, the source CPU would only sync its cache when the lock is actually
 dropped by the outer xnlock_put_irq* call and the inner
 xnlock_get/put_irq* would not act as barriers, so the remote
 rescheduling handler won't always see the XNSCHED update done remotely,
 and may lead to a no-op. So we need a barrier before sending the IPI in
 __xnpod_test_resched().

 That's what I said.

 And we need it on the reader side as an rmb().
 
 This one we have, in xnpod_schedule_handler.
 

Right, with your patch (the above sounded like we only need it on writer
side).

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-07 Thread Philippe Gerum

On Sun, 2010-11-07 at 11:14 +0100, Jan Kiszka wrote:
 Am 07.11.2010 11:12, Gilles Chanteperdrix wrote:
  Jan Kiszka wrote:
  Am 07.11.2010 11:03, Philippe Gerum wrote:
  On Sun, 2010-11-07 at 09:31 +0100, Gilles Chanteperdrix wrote:
  Jan Kiszka wrote:
  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
  Check the kernel, we actually need it on both sides. Wherever the final
  barriers will be, we should leave a comment behind why they are there.
  Could be picked up from kernel/smp.c.
  We have it on both sides: the non-local flags are modified while holding
  the nklock. Unlocking the nklock implies a barrier.
  I think we may have an issue with this kind of construct:
 
  xnlock_get_irq*(nklock)
xnpod_resume/suspend/whatever_thread()
xnlock_get_irq*(nklock)
...
xnlock_put_irq*(nklock)
xnpod_schedule()
xnlock_get_irq*(nklock)
send_ipi
= xnpod_schedule_handler on dest CPU
xnlock_put_irq*(nklock)
  xnlock_put_irq*(nklock)
 
  The issue would be triggered by the use of recursive locking. In that
  case, the source CPU would only sync its cache when the lock is actually
  dropped by the outer xnlock_put_irq* call and the inner
  xnlock_get/put_irq* would not act as barriers, so the remote
  rescheduling handler won't always see the XNSCHED update done remotely,
  and may lead to a no-op. So we need a barrier before sending the IPI in
  __xnpod_test_resched().
 
  That's what I said.
 
  And we need it on the reader side as an rmb().
  
  This one we have, in xnpod_schedule_handler.
  
 
 Right, with your patch (the above sounded like we only need it on writer
 side).

C'mon...

-- 
Philippe.



___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-06 Thread Anders Blomdell


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 cf4..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

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-06 Thread Gilles Chanteperdrix

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 cf4..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,

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-06 Thread Philippe Gerum

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 cf4..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

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-06 Thread Jan Kiszka

Am 06.11.2010 23:49, Philippe Gerum wrote:
 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 cf4..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) {
 +

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-05 Thread Anders Blomdell


Gilles Chanteperdrix wrote:

Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 05.11.2010 00:25, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
debugging off.

That is not enough.

It is, I've reviewed the code today.

The fallouts I am talking about are:
47dac49c71e89b684203e854d1b0172ecacbc555

Not related.


38f2ca83a8e63cc94eaa911ff1c0940c884b5078

An optimization.


5e7cfa5c25672e4478a721eadbd6f6c5b4f88a2f

That fall out of that commit is fixed in my series.


This commit was followed by several others to fix
the fix. You know how things are, someone proposes a fix, which fixes
things for him, but it breaks in the other people configurations (one of
the fallouts was a complete revamp of include/asm-arm/atomic.h for
instance).


I've pushed a series that reverts that commit, then fixes and cleans up
on top of it. Just pushed if you want to take a look. We can find some
alternative debugging mechanism independently (though I'm curious to see
it - it still makes no sense to me).

Since the fix is simply a modification to what we have currently. I
would prefer if we did not remove it. In fact, I think it would be
simpler if we started from what we currently have than reverting past
patches.

Look at the series, it goes step by step to an IMHO clean state. We can
pull out the debugging check removal, though, if you prefer to work on
top of the existing code.
From my point of view, Anders looks for something that works, so 
following the rules that the minimal set of changes minimize the chances
of introducing new bugs while cleaning, I would go for the minimal set 
of changes, such as:


The tested one (on SMP, and UP with and without unlocked ctx switch):

diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..cf4 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -165,28 +165,27 @@ struct xnsched_class {
 #endif /* CONFIG_SMP */
 
 /* Test all resched flags from the given scheduler mask. */

-static inline int xnsched_resched_p(struct xnsched *sched)
+static inline int xnsched_remote_resched_p(struct xnsched *sched)
 {
-   return testbits(sched-status, XNRESCHED);
+   return !xnarch_cpus_empty(sched-resched);
 }
 
-static inline int xnsched_self_resched_p(struct xnsched *sched)

+static inline int xnsched_resched_p(struct xnsched *sched)
 {
return testbits(sched-status, XNRESCHED);
 }
 
 /* Set self resched flag for the given scheduler. */

 #define xnsched_set_self_resched(__sched__) do {   \
-  setbits((__sched__)-status, XNRESCHED);  \
+  __setbits((__sched__)-status, XNRESCHED);\
 } while (0)
 
 /* Set specific resched flag into the local scheduler mask. */

 #define xnsched_set_resched(__sched__) do {\
   xnsched_t *current_sched = xnpod_current_sched();\
-  setbits(current_sched-status, XNRESCHED);\
+  __setbits(current_sched-status, XNRESCHED);  \
   if (current_sched != (__sched__)){   \
   xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);   \
-  setbits((__sched__)-status, XNRESCHED);  \
   }\
 } while (0)
 
diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c

index 862838c..4cb707a 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -276,18 +276,16 @@ EXPORT_SYMBOL_GPL(xnpod_fatal_helper);
 
 void xnpod_schedule_handler(void) /* Called with hw interrupts off. */

 {
-   xnsched_t *sched;
+   xnsched_t *sched = xnpod_current_sched();
 
 	trace_mark(xn_nucleus, sched_remote, MARK_NOARGS);

 #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
-   sched = xnpod_current_sched();
if (testbits(sched-status, XNRPICK)) {
clrbits(sched-status, XNRPICK);
xnshadow_rpi_check();
}
-#else
-   (void)sched;
 #endif /* CONFIG_SMP  CONFIG_XENO_OPT_PRIOCPL */
+   xnsched_set_self_resched(sched);
xnpod_schedule();
 }
 
@@ -2174,7 +2172,7 @@ static inline int __xnpod_test_resched(struct xnsched *sched)

int resched = testbits(sched-status, XNRESCHED);
 #ifdef CONFIG_SMP
/* Send resched IPI to remote CPU(s). */
-   if (unlikely(xnsched_resched_p(sched))) {
+   if (unlikely(xnsched_remote_resched_p(sched))) {
xnarch_send_ipi(sched-resched);
xnarch_cpus_clear(sched-resched);
}
diff --git a/ksrc/nucleus/timer.c b/ksrc/nucleus/timer.c
index 1fe3331..a0ac627 100644
--- a/ksrc/nucleus/timer.c
+++ b/ksrc/nucleus/timer.c
@@ -97,7 +97,7 @@ void xntimer_next_local_shot(xnsched_t *sched)
__clrbits(sched-status, XNHDEFER);
timer = aplink2timer(h);

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-05 Thread Anders Blomdell


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 cf4..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

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-05 Thread Gilles Chanteperdrix

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 cf4..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.

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 01:13, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:56, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:44, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits 
 (but
 we do in other cases where this is no need - different story). 
 This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of 
 nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
   * context is active, or if we are caught in the middle of a
   * unlocked context switch.
   */
 -#if XENO_DEBUG(NUCLEUS)
  if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
  return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -if (testbits(sched-status,
 - XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +if (!sched-resched)
  return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?

 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when folding all 
 bits
 into a single word). And that should be much more costly, specifically
 on SMP.
 What about issuing a barrier before testing the status?

 The problem is not about reading but writing the status concurrently,
 thus it's not about the code you see above.
 The bits are modified under nklock, which implies a barrier when
 unlocked. Furthermore, an IPI is guaranteed to be received on the remote
 CPU after this barrier, so, a barrier should be enough to see the
 modifications which have been made remotely.
 Check nucleus/intr.c for tons of unprotected status modifications.
 Ok. Then maybe, we should reconsider the original decision to start
 fiddling with the XNRESCHED bit remotely.

 ...which removed complexity and fixed a race? Let's better review the
 checks done in xnpod_schedule vs. its callers, I bet there is more to
 save (IOW: remove the need to test for sched-resched).
 
 Not that much complexitiy... and the race was a false positive in debug
 code, no big deal. At least it worked, and it has done so for a long
 time. No atomic needed, no barrier, only one test in xnpod_schedule. And
 a nice invariant: sched-status is always accessed on the local cpu.
 What else?

Take a step back and look at the root cause for this issue again. Unlocked

if need-resched
__xnpod_schedule

is inherently racy and will always be (not only for the remote
reschedule case BTW). So we either have to accept this and remove the
debugging check from the scheduler or push the check back to
__xnpod_schedule where it once came from. When this it cleaned up, we
can look into the remote resched protocol again.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100


Jan Kiszka wrote:

Am 04.11.2010 01:13, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 04.11.2010 00:56, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 04.11.2010 00:44, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 03.11.2010 23:11, Jan Kiszka wrote:

Am 03.11.2010 23:03, Jan Kiszka wrote:

But we not not always use atomic ops for manipulating status bits (but
we do in other cases where this is no need - different story). This may
fix the race:

Err, nonsense. As we manipulate xnsched::status also outside of nklock
protection, we must _always_ use atomic ops.

This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
should be pushed in a separate status word that can then be safely
modified non-atomically.

Second try to fix and clean up the sched status bits. Anders, please
test.

Jan

diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
index 01ff0a7..5987a1f 100644
--- a/include/nucleus/pod.h
+++ b/include/nucleus/pod.h
@@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
 * context is active, or if we are caught in the middle of a
 * unlocked context switch.
 */
-#if XENO_DEBUG(NUCLEUS)
if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
return;
-#else /* !XENO_DEBUG(NUCLEUS) */
-   if (testbits(sched-status,
-XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
+#if !XENO_DEBUG(NUCLEUS)
+   if (!sched-resched)
return;
 #endif /* !XENO_DEBUG(NUCLEUS) */

Having only one test was really nice here, maybe we simply read a
barrier before reading the status?


I agree - but the alternative is letting all modifications of
xnsched::status use atomic bitops (that's required when folding all bits
into a single word). And that should be much more costly, specifically
on SMP.

What about issuing a barrier before testing the status?


The problem is not about reading but writing the status concurrently,
thus it's not about the code you see above.

The bits are modified under nklock, which implies a barrier when
unlocked. Furthermore, an IPI is guaranteed to be received on the remote
CPU after this barrier, so, a barrier should be enough to see the
modifications which have been made remotely.

Check nucleus/intr.c for tons of unprotected status modifications.

Ok. Then maybe, we should reconsider the original decision to start
fiddling with the XNRESCHED bit remotely.

...which removed complexity and fixed a race? Let's better review the
checks done in xnpod_schedule vs. its callers, I bet there is more to
save (IOW: remove the need to test for sched-resched).

Not that much complexitiy... and the race was a false positive in debug
code, no big deal. At least it worked, and it has done so for a long
time. No atomic needed, no barrier, only one test in xnpod_schedule. And
a nice invariant: sched-status is always accessed on the local cpu.
What else?


Take a step back and look at the root cause for this issue again. Unlocked

if need-resched
__xnpod_schedule

is inherently racy and will always be (not only for the remote
reschedule case BTW). So we either have to accept this and remove the
debugging check from the scheduler or push the check back to
__xnpod_schedule where it once came from. When this it cleaned up, we
can look into the remote resched protocol again.
Probably being daft here; why not stop fiddling with remote CPU status 
bits and always do a reschedule on IPI irq's?


/Anders


___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 09:45, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 01:13, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:56, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:44, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating
 status bits (but
 we do in other cases where this is no need - different
 story). This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also
 outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or
 XNINIRQ
 should be pushed in a separate status word that can then be
 safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits.
 Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
   * context is active, or if we are caught in the middle
 of a
   * unlocked context switch.
   */
 -#if XENO_DEBUG(NUCLEUS)
  if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
  return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -if (testbits(sched-status,
 - XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +if (!sched-resched)
  return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply
 read a
 barrier before reading the status?

 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when
 folding all bits
 into a single word). And that should be much more costly,
 specifically
 on SMP.
 What about issuing a barrier before testing the status?

 The problem is not about reading but writing the status
 concurrently,
 thus it's not about the code you see above.
 The bits are modified under nklock, which implies a barrier when
 unlocked. Furthermore, an IPI is guaranteed to be received on the
 remote
 CPU after this barrier, so, a barrier should be enough to see the
 modifications which have been made remotely.
 Check nucleus/intr.c for tons of unprotected status modifications.
 Ok. Then maybe, we should reconsider the original decision to start
 fiddling with the XNRESCHED bit remotely.
 ...which removed complexity and fixed a race? Let's better review the
 checks done in xnpod_schedule vs. its callers, I bet there is more to
 save (IOW: remove the need to test for sched-resched).
 Not that much complexitiy... and the race was a false positive in debug
 code, no big deal. At least it worked, and it has done so for a long
 time. No atomic needed, no barrier, only one test in xnpod_schedule. And
 a nice invariant: sched-status is always accessed on the local cpu.
 What else?

 Take a step back and look at the root cause for this issue again.
 Unlocked

 if need-resched
 __xnpod_schedule

 is inherently racy and will always be (not only for the remote
 reschedule case BTW). So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.
 Probably being daft here; why not stop fiddling with remote CPU status
 bits and always do a reschedule on IPI irq's?

Yes that's what we did before. But the snippet above was and still is
the issue to be resolved first. It motivated the change in the remote
reschedule to avoid one of the possible races around this check.

Jan

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

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Take a step back and look at the root cause for this issue again. Unlocked
 
   if need-resched
   __xnpod_schedule
 
 is inherently racy and will always be (not only for the remote
 reschedule case BTW).

Ok, let us examine what may happen with this code if we only set the
XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
matter, because they can not change under our feet. So, we have two
cases for this race:
1- we see the XNRESCHED bit, but it has been cleared once nklock is
locked in __xnpod_schedule.
2- we do not see the XNRESCHED bit, but it get set right after we test it.

1 is not a problem.
2 is not a problem, because anything which sets the XNRESCHED (it may
only be an interrupt in fact) bit will cause xnpod_schedule to be called
right after that.

So no, no race here provided that we only set the XNRESCHED bit on the
local cpu.

 So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.

The problem of the debug check is that it checks whether the scheduler
state is modified without the XNRESCHED bit being set. And this is the
problem, because yes, in that case, we have a race: the scheduler state
may be modified before the XNRESCHED bit is set by an IPI.

If we want to fix the debug check, we have to have a special bit, on in
the sched-status flag, only for the purpose of debugging. Or remove the
debug check.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Anders Blomdell wrote:
 Probably being daft here; why not stop fiddling with remote CPU status 
 bits and always do a reschedule on IPI irq's?

That is what we had been doing for a long time, and stopped between
2.5.4 and 2.5.5, and this is what I say: maybe this was not such a good
idea.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Take a step back and look at the root cause for this issue again. Unlocked

  if need-resched
  __xnpod_schedule

 is inherently racy and will always be (not only for the remote
 reschedule case BTW).
 
 Ok, let us examine what may happen with this code if we only set the
 XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
 matter, because they can not change under our feet. So, we have two
 cases for this race:
 1- we see the XNRESCHED bit, but it has been cleared once nklock is
 locked in __xnpod_schedule.
 2- we do not see the XNRESCHED bit, but it get set right after we test it.
 
 1 is not a problem.
 2 is not a problem, because anything which sets the XNRESCHED (it may
 only be an interrupt in fact) bit will cause xnpod_schedule to be called
 right after that.
 
 So no, no race here provided that we only set the XNRESCHED bit on the
 local cpu.
 
  So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.
 
 The problem of the debug check is that it checks whether the scheduler
 state is modified without the XNRESCHED bit being set. And this is the
 problem, because yes, in that case, we have a race: the scheduler state
 may be modified before the XNRESCHED bit is set by an IPI.
 
 If we want to fix the debug check, we have to have a special bit, on in
   NOT
 the sched-status flag, only for the purpose of debugging. Or remove the
 debug check.
 


-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Take a step back and look at the root cause for this issue again. Unlocked

  if need-resched
  __xnpod_schedule

 is inherently racy and will always be (not only for the remote
 reschedule case BTW).
 
 Ok, let us examine what may happen with this code if we only set the
 XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
 matter, because they can not change under our feet. So, we have two
 cases for this race:
 1- we see the XNRESCHED bit, but it has been cleared once nklock is
 locked in __xnpod_schedule.
 2- we do not see the XNRESCHED bit, but it get set right after we test it.
 
 1 is not a problem.

Yes, as long as we remove the debug check from the scheduler code (or
fix it somehow). The scheduling code already catches this race.

 2 is not a problem, because anything which sets the XNRESCHED (it may
 only be an interrupt in fact) bit will cause xnpod_schedule to be called
 right after that.
 
 So no, no race here provided that we only set the XNRESCHED bit on the
 local cpu.
 
  So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.
 
 The problem of the debug check is that it checks whether the scheduler
 state is modified without the XNRESCHED bit being set. And this is the
 problem, because yes, in that case, we have a race: the scheduler state
 may be modified before the XNRESCHED bit is set by an IPI.
 
 If we want to fix the debug check, we have to have a special bit, on in
 the sched-status flag, only for the purpose of debugging. Or remove the
 debug check.

Exactly my point. Is there any benefit in keeping the debug check? The
code to make it work may end up as complex as the logic it verifies,
at least that's my current feeling.

Jan

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

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 10:26, Jan Kiszka wrote:
 Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Take a step back and look at the root cause for this issue again. Unlocked

 if need-resched
 __xnpod_schedule

 is inherently racy and will always be (not only for the remote
 reschedule case BTW).

 Ok, let us examine what may happen with this code if we only set the
 XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
 matter, because they can not change under our feet. So, we have two
 cases for this race:
 1- we see the XNRESCHED bit, but it has been cleared once nklock is
 locked in __xnpod_schedule.
 2- we do not see the XNRESCHED bit, but it get set right after we test it.

 1 is not a problem.
 
 Yes, as long as we remove the debug check from the scheduler code (or
 fix it somehow). The scheduling code already catches this race.
 
 2 is not a problem, because anything which sets the XNRESCHED (it may
 only be an interrupt in fact) bit will cause xnpod_schedule to be called
 right after that.

 So no, no race here provided that we only set the XNRESCHED bit on the
 local cpu.

  So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.

 The problem of the debug check is that it checks whether the scheduler
 state is modified without the XNRESCHED bit being set. And this is the
 problem, because yes, in that case, we have a race: the scheduler state
 may be modified before the XNRESCHED bit is set by an IPI.

 If we want to fix the debug check, we have to have a special bit, on in
 the sched-status flag, only for the purpose of debugging. Or remove the
 debug check.
 
 Exactly my point. Is there any benefit in keeping the debug check? The
 code to make it work may end up as complex as the logic it verifies,
 at least that's my current feeling.
 

This would be the radical approach of removing the check (and cleaning
up some bits). If it's acceptable, I would split it up properly.

diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
index 01ff0a7..71f8311 100644
--- a/include/nucleus/pod.h
+++ b/include/nucleus/pod.h
@@ -277,14 +277,9 @@ static inline void xnpod_schedule(void)
 * context is active, or if we are caught in the middle of a
 * unlocked context switch.
 */
-#if XENO_DEBUG(NUCLEUS)
-   if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
-   return;
-#else /* !XENO_DEBUG(NUCLEUS) */
if (testbits(sched-status,
 XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
return;
-#endif /* !XENO_DEBUG(NUCLEUS) */
 
__xnpod_schedule(sched);
 }
diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..c832b91 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -177,17 +177,16 @@ static inline int xnsched_self_resched_p(struct xnsched 
*sched)
 
 /* Set self resched flag for the given scheduler. */
 #define xnsched_set_self_resched(__sched__) do {   \
-  setbits((__sched__)-status, XNRESCHED); \
+   __setbits((__sched__)-status, XNRESCHED);  \
 } while (0)
 
 /* Set specific resched flag into the local scheduler mask. */
 #define xnsched_set_resched(__sched__) do {\
-  xnsched_t *current_sched = xnpod_current_sched();\
-  setbits(current_sched-status, XNRESCHED);   \
-  if (current_sched != (__sched__)){   \
-  xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);  \
-  setbits((__sched__)-status, XNRESCHED); \
-  }\
+   xnsched_t *current_sched = xnpod_current_sched();   \
+   __setbits(current_sched-status, XNRESCHED);\
+   if (current_sched != (__sched__))   \
+   xnarch_cpu_set(xnsched_cpu(__sched__),  \
+  current_sched-resched); \
 } while (0)
 
 void xnsched_zombie_hooks(struct xnthread *thread);
diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
index 9e135f3..87dc136 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -284,10 +284,11 @@ void xnpod_schedule_handler(void) /* Called with hw 
interrupts off. */
trace_xn_nucleus_sched_remote(sched);
 #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
if (testbits(sched-status, XNRPICK)) {
-   clrbits(sched-status, XNRPICK);
+   __clrbits(sched-status, XNRPICK);
xnshadow_rpi_check();
}
 #endif /* CONFIG_SMP  CONFIG_XENO_OPT_PRIOCPL */
+   xnsched_set_resched(sched);
xnpod_schedule();
 }
 
@@ -2162,21 +2163,21 @@

Re: [Xenomai-core] Potential problem with rt_eepro100


Jan Kiszka wrote:

Am 04.11.2010 10:26, Jan Kiszka wrote:

Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Take a step back and look at the root cause for this issue again. Unlocked

if need-resched
__xnpod_schedule

is inherently racy and will always be (not only for the remote
reschedule case BTW).

Ok, let us examine what may happen with this code if we only set the
XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
matter, because they can not change under our feet. So, we have two
cases for this race:
1- we see the XNRESCHED bit, but it has been cleared once nklock is
locked in __xnpod_schedule.
2- we do not see the XNRESCHED bit, but it get set right after we test it.

1 is not a problem.

Yes, as long as we remove the debug check from the scheduler code (or
fix it somehow). The scheduling code already catches this race.


2 is not a problem, because anything which sets the XNRESCHED (it may
only be an interrupt in fact) bit will cause xnpod_schedule to be called
right after that.

So no, no race here provided that we only set the XNRESCHED bit on the
local cpu.

 So we either have to accept this and remove the

debugging check from the scheduler or push the check back to
__xnpod_schedule where it once came from. When this it cleaned up, we
can look into the remote resched protocol again.

The problem of the debug check is that it checks whether the scheduler
state is modified without the XNRESCHED bit being set. And this is the
problem, because yes, in that case, we have a race: the scheduler state
may be modified before the XNRESCHED bit is set by an IPI.

If we want to fix the debug check, we have to have a special bit, on in
the sched-status flag, only for the purpose of debugging. Or remove the
debug check.

Exactly my point. Is there any benefit in keeping the debug check? The
code to make it work may end up as complex as the logic it verifies,
at least that's my current feeling.



This would be the radical approach of removing the check (and cleaning
up some bits). If it's acceptable, I would split it up properly.

diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
index 01ff0a7..71f8311 100644
--- a/include/nucleus/pod.h
+++ b/include/nucleus/pod.h
@@ -277,14 +277,9 @@ static inline void xnpod_schedule(void)
 * context is active, or if we are caught in the middle of a
 * unlocked context switch.
 */
-#if XENO_DEBUG(NUCLEUS)
-   if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
-   return;
-#else /* !XENO_DEBUG(NUCLEUS) */
if (testbits(sched-status,
 XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
return;
-#endif /* !XENO_DEBUG(NUCLEUS) */
 
 	__xnpod_schedule(sched);

 }
diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..c832b91 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -177,17 +177,16 @@ static inline int xnsched_self_resched_p(struct xnsched 
*sched)
 
 /* Set self resched flag for the given scheduler. */

 #define xnsched_set_self_resched(__sched__) do {   \
-  setbits((__sched__)-status, XNRESCHED);  \
+   __setbits((__sched__)-status, XNRESCHED);   \
 } while (0)
 
 /* Set specific resched flag into the local scheduler mask. */

 #define xnsched_set_resched(__sched__) do {\
-  xnsched_t *current_sched = xnpod_current_sched();\
-  setbits(current_sched-status, XNRESCHED);\
-  if (current_sched != (__sched__)){   \
-  xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);   \
-  setbits((__sched__)-status, XNRESCHED);  \
-  }\
+   xnsched_t *current_sched = xnpod_current_sched();   \
+   __setbits(current_sched-status, XNRESCHED); \
+   if (current_sched != (__sched__))   \
+   xnarch_cpu_set(xnsched_cpu(__sched__),  \
+  current_sched-resched);  \
 } while (0)
 
 void xnsched_zombie_hooks(struct xnthread *thread);

diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
index 9e135f3..87dc136 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -284,10 +284,11 @@ void xnpod_schedule_handler(void) /* Called with hw 
interrupts off. */
trace_xn_nucleus_sched_remote(sched);
 #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
if (testbits(sched-status, XNRPICK)) {
-   clrbits(sched-status, XNRPICK);
+   __clrbits(sched-status, XNRPICK);
xnshadow_rpi_check();
}
 #endif /* CONFIG_SMP  CONFIG_XENO_OPT_PRIOCPL */
+   xnsched_set_resched(sched);
xnpod_schedule();
 }
 
@@ -2162,21 +2163,21

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 04.11.2010 10:26, Jan Kiszka wrote:
 Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Take a step back and look at the root cause for this issue again. Unlocked

if need-resched
__xnpod_schedule

 is inherently racy and will always be (not only for the remote
 reschedule case BTW).
 Ok, let us examine what may happen with this code if we only set the
 XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
 matter, because they can not change under our feet. So, we have two
 cases for this race:
 1- we see the XNRESCHED bit, but it has been cleared once nklock is
 locked in __xnpod_schedule.
 2- we do not see the XNRESCHED bit, but it get set right after we test it.

 1 is not a problem.
 Yes, as long as we remove the debug check from the scheduler code (or
 fix it somehow). The scheduling code already catches this race.

 2 is not a problem, because anything which sets the XNRESCHED (it may
 only be an interrupt in fact) bit will cause xnpod_schedule to be called
 right after that.

 So no, no race here provided that we only set the XNRESCHED bit on the
 local cpu.

  So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.
 The problem of the debug check is that it checks whether the scheduler
 state is modified without the XNRESCHED bit being set. And this is the
 problem, because yes, in that case, we have a race: the scheduler state
 may be modified before the XNRESCHED bit is set by an IPI.

 If we want to fix the debug check, we have to have a special bit, on in
 the sched-status flag, only for the purpose of debugging. Or remove the
 debug check.
 Exactly my point. Is there any benefit in keeping the debug check? The
 code to make it work may end up as complex as the logic it verifies,
 at least that's my current feeling.

 
 This would be the radical approach of removing the check (and cleaning
 up some bits). If it's acceptable, I would split it up properly.

This debug check saved our asses when debugging SMP issues, and I
suspect it may help debugging skin issues. So, I think we should try and
keep it.

 @@ -2162,21 +2163,21 @@ static inline void xnpod_switch_to(xnsched_t *sched,
  
  static inline int __xnpod_test_resched(struct xnsched *sched)
  {
 - int resched = testbits(sched-status, XNRESCHED);
 + int resched = xnsched_resched_p(sched);
  #ifdef CONFIG_SMP
   /* Send resched IPI to remote CPU(s). */
 - if (unlikely(xnsched_resched_p(sched))) {
 + if (unlikely(resched)) {

At first sight, here you are more breaking things than cleaning them.


-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100


Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 04.11.2010 10:26, Jan Kiszka wrote:

Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Take a step back and look at the root cause for this issue again. Unlocked

if need-resched
__xnpod_schedule

is inherently racy and will always be (not only for the remote
reschedule case BTW).

Ok, let us examine what may happen with this code if we only set the
XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
matter, because they can not change under our feet. So, we have two
cases for this race:
1- we see the XNRESCHED bit, but it has been cleared once nklock is
locked in __xnpod_schedule.
2- we do not see the XNRESCHED bit, but it get set right after we test it.

1 is not a problem.

Yes, as long as we remove the debug check from the scheduler code (or
fix it somehow). The scheduling code already catches this race.


2 is not a problem, because anything which sets the XNRESCHED (it may
only be an interrupt in fact) bit will cause xnpod_schedule to be called
right after that.

So no, no race here provided that we only set the XNRESCHED bit on the
local cpu.

 So we either have to accept this and remove the

debugging check from the scheduler or push the check back to
__xnpod_schedule where it once came from. When this it cleaned up, we
can look into the remote resched protocol again.

The problem of the debug check is that it checks whether the scheduler
state is modified without the XNRESCHED bit being set. And this is the
problem, because yes, in that case, we have a race: the scheduler state
may be modified before the XNRESCHED bit is set by an IPI.

If we want to fix the debug check, we have to have a special bit, on in
the sched-status flag, only for the purpose of debugging. Or remove the
debug check.

Exactly my point. Is there any benefit in keeping the debug check? The
code to make it work may end up as complex as the logic it verifies,
at least that's my current feeling.


This would be the radical approach of removing the check (and cleaning
up some bits). If it's acceptable, I would split it up properly.


This debug check saved our asses when debugging SMP issues, and I
suspect it may help debugging skin issues. So, I think we should try and
keep it.


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).


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)?


/Anders


___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 14:18, Anders Blomdell wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 10:26, Jan Kiszka wrote:
 Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Take a step back and look at the root cause for this issue again. 
 Unlocked

  if need-resched
  __xnpod_schedule

 is inherently racy and will always be (not only for the remote
 reschedule case BTW).
 Ok, let us examine what may happen with this code if we only set the
 XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
 matter, because they can not change under our feet. So, we have two
 cases for this race:
 1- we see the XNRESCHED bit, but it has been cleared once nklock is
 locked in __xnpod_schedule.
 2- we do not see the XNRESCHED bit, but it get set right after we test it.

 1 is not a problem.
 Yes, as long as we remove the debug check from the scheduler code (or
 fix it somehow). The scheduling code already catches this race.

 2 is not a problem, because anything which sets the XNRESCHED (it may
 only be an interrupt in fact) bit will cause xnpod_schedule to be called
 right after that.

 So no, no race here provided that we only set the XNRESCHED bit on the
 local cpu.

  So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.
 The problem of the debug check is that it checks whether the scheduler
 state is modified without the XNRESCHED bit being set. And this is the
 problem, because yes, in that case, we have a race: the scheduler state
 may be modified before the XNRESCHED bit is set by an IPI.

 If we want to fix the debug check, we have to have a special bit, on in
 the sched-status flag, only for the purpose of debugging. Or remove the
 debug check.
 Exactly my point. Is there any benefit in keeping the debug check? The
 code to make it work may end up as complex as the logic it verifies,
 at least that's my current feeling.

 This would be the radical approach of removing the check (and cleaning
 up some bits). If it's acceptable, I would split it up properly.

 This debug check saved our asses when debugging SMP issues, and I
 suspect it may help debugging skin issues. So, I think we should try and
 keep it.


 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.

Jan

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

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100


Jan Kiszka wrote:

Am 04.11.2010 14:18, Anders Blomdell wrote:

Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Am 04.11.2010 10:26, Jan Kiszka wrote:

Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:

Jan Kiszka wrote:

Take a step back and look at the root cause for this issue again. Unlocked

if need-resched
__xnpod_schedule

is inherently racy and will always be (not only for the remote
reschedule case BTW).

Ok, let us examine what may happen with this code if we only set the
XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
matter, because they can not change under our feet. So, we have two
cases for this race:
1- we see the XNRESCHED bit, but it has been cleared once nklock is
locked in __xnpod_schedule.
2- we do not see the XNRESCHED bit, but it get set right after we test it.

1 is not a problem.

Yes, as long as we remove the debug check from the scheduler code (or
fix it somehow). The scheduling code already catches this race.


2 is not a problem, because anything which sets the XNRESCHED (it may
only be an interrupt in fact) bit will cause xnpod_schedule to be called
right after that.

So no, no race here provided that we only set the XNRESCHED bit on the
local cpu.

 So we either have to accept this and remove the

debugging check from the scheduler or push the check back to
__xnpod_schedule where it once came from. When this it cleaned up, we
can look into the remote resched protocol again.

The problem of the debug check is that it checks whether the scheduler
state is modified without the XNRESCHED bit being set. And this is the
problem, because yes, in that case, we have a race: the scheduler state
may be modified before the XNRESCHED bit is set by an IPI.

If we want to fix the debug check, we have to have a special bit, on in
the sched-status flag, only for the purpose of debugging. Or remove the
debug check.

Exactly my point. Is there any benefit in keeping the debug check? The
code to make it work may end up as complex as the logic it verifies,
at least that's my current feeling.


This would be the radical approach of removing the check (and cleaning
up some bits). If it's acceptable, I would split it up properly.

This debug check saved our asses when debugging SMP issues, and I
suspect it may help debugging skin issues. So, I think we should try and
keep it.


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.
Any reason why the two changes below would fail (I need to get things 
working real soon now).


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 only thing I can think of are atomic set/clear on an independent 
variable.


/Anders

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 15:53, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 14:18, Anders Blomdell wrote:
 Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 10:26, Jan Kiszka wrote:
 Am 04.11.2010 10:16, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Take a step back and look at the root cause for this issue again. 
 Unlocked

if need-resched
__xnpod_schedule

 is inherently racy and will always be (not only for the remote
 reschedule case BTW).
 Ok, let us examine what may happen with this code if we only set the
 XNRESCHED bit on the local cpu. First, other bits than XNRESCHED do not
 matter, because they can not change under our feet. So, we have two
 cases for this race:
 1- we see the XNRESCHED bit, but it has been cleared once nklock is
 locked in __xnpod_schedule.
 2- we do not see the XNRESCHED bit, but it get set right after we test 
 it.

 1 is not a problem.
 Yes, as long as we remove the debug check from the scheduler code (or
 fix it somehow). The scheduling code already catches this race.

 2 is not a problem, because anything which sets the XNRESCHED (it may
 only be an interrupt in fact) bit will cause xnpod_schedule to be called
 right after that.

 So no, no race here provided that we only set the XNRESCHED bit on the
 local cpu.

  So we either have to accept this and remove the
 debugging check from the scheduler or push the check back to
 __xnpod_schedule where it once came from. When this it cleaned up, we
 can look into the remote resched protocol again.
 The problem of the debug check is that it checks whether the scheduler
 state is modified without the XNRESCHED bit being set. And this is the
 problem, because yes, in that case, we have a race: the scheduler state
 may be modified before the XNRESCHED bit is set by an IPI.

 If we want to fix the debug check, we have to have a special bit, on in
 the sched-status flag, only for the purpose of debugging. Or remove the
 debug check.
 Exactly my point. Is there any benefit in keeping the debug check? The
 code to make it work may end up as complex as the logic it verifies,
 at least that's my current feeling.

 This would be the radical approach of removing the check (and cleaning
 up some bits). If it's acceptable, I would split it up properly.
 This debug check saved our asses when debugging SMP issues, and I
 suspect it may help debugging skin issues. So, I think we should try and
 keep it.


 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.
 Any reason why the two changes below would fail (I need to get things 
 working real soon now).

Maybe they don't fail but definitely cause reschedules where we do not
need them. I stopped thinking about those details after starting the
rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
debugging off.


 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 only thing I can think of are atomic set/clear on an independent 
 variable.

And the set has to be confined to few central locations - otherwise it
will be simpler to check for the proper pairing of runqueue manipulation
and xnsched_set_resched manually - if that was the fault pattern the
test was supposed to catch (which would have had nucleus scope, no skins
involved).

Jan

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

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

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.
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 will try and work on this this week-end.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.

That is not enough. This commit was followed by several others to fix
the fix. You know how things are, someone proposes a fix, which fixes
things for him, but it breaks in the other people configurations (one of
the fallouts was a complete revamp of include/asm-arm/atomic.h for
instance).

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.
 
 That is not enough.

It is, I've reviewed the code today.

 This commit was followed by several others to fix
 the fix. You know how things are, someone proposes a fix, which fixes
 things for him, but it breaks in the other people configurations (one of
 the fallouts was a complete revamp of include/asm-arm/atomic.h for
 instance).
 

I've pushed a series that reverts that commit, then fixes and cleans up
on top of it. Just pushed if you want to take a look. We can find some
alternative debugging mechanism independently (though I'm curious to see
it - it still makes no sense to me).

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

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).

 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?

But maybe you can provide some motivating bug scenarios, real ones of
the past or realistic ones of the future.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

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.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.
 That is not enough.
 
 It is, I've reviewed the code today.

The fallouts I am talking about are:
47dac49c71e89b684203e854d1b0172ecacbc555
38f2ca83a8e63cc94eaa911ff1c0940c884b5078
5e7cfa5c25672e4478a721eadbd6f6c5b4f88a2f


 
 This commit was followed by several others to fix
 the fix. You know how things are, someone proposes a fix, which fixes
 things for him, but it breaks in the other people configurations (one of
 the fallouts was a complete revamp of include/asm-arm/atomic.h for
 instance).

 
 I've pushed a series that reverts that commit, then fixes and cleans up
 on top of it. Just pushed if you want to take a look. We can find some
 alternative debugging mechanism independently (though I'm curious to see
 it - it still makes no sense to me).

Since the fix is simply a modification to what we have currently. I
would prefer if we did not remove it. In fact, I think it would be
simpler if we started from what we currently have than reverting past
patches.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 05.11.2010 00:25, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.
 That is not enough.

 It is, I've reviewed the code today.
 
 The fallouts I am talking about are:
 47dac49c71e89b684203e854d1b0172ecacbc555

Not related.

 38f2ca83a8e63cc94eaa911ff1c0940c884b5078

An optimization.

 5e7cfa5c25672e4478a721eadbd6f6c5b4f88a2f

That fall out of that commit is fixed in my series.

 

 This commit was followed by several others to fix
 the fix. You know how things are, someone proposes a fix, which fixes
 things for him, but it breaks in the other people configurations (one of
 the fallouts was a complete revamp of include/asm-arm/atomic.h for
 instance).


 I've pushed a series that reverts that commit, then fixes and cleans up
 on top of it. Just pushed if you want to take a look. We can find some
 alternative debugging mechanism independently (though I'm curious to see
 it - it still makes no sense to me).
 
 Since the fix is simply a modification to what we have currently. I
 would prefer if we did not remove it. In fact, I think it would be
 simpler if we started from what we currently have than reverting past
 patches.

Look at the series, it goes step by step to an IMHO clean state. We can
pull out the debugging check removal, though, if you prefer to work on
top of the existing code.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

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.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 05.11.2010 00:25, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.
 That is not enough.
 It is, I've reviewed the code today.
 The fallouts I am talking about are:
 47dac49c71e89b684203e854d1b0172ecacbc555
 
 Not related.
 
 38f2ca83a8e63cc94eaa911ff1c0940c884b5078
 
 An optimization.
 
 5e7cfa5c25672e4478a721eadbd6f6c5b4f88a2f
 
 That fall out of that commit is fixed in my series.
 
 This commit was followed by several others to fix
 the fix. You know how things are, someone proposes a fix, which fixes
 things for him, but it breaks in the other people configurations (one of
 the fallouts was a complete revamp of include/asm-arm/atomic.h for
 instance).

 I've pushed a series that reverts that commit, then fixes and cleans up
 on top of it. Just pushed if you want to take a look. We can find some
 alternative debugging mechanism independently (though I'm curious to see
 it - it still makes no sense to me).
 Since the fix is simply a modification to what we have currently. I
 would prefer if we did not remove it. In fact, I think it would be
 simpler if we started from what we currently have than reverting past
 patches.
 
 Look at the series, it goes step by step to an IMHO clean state. We can
 pull out the debugging check removal, though, if you prefer to work on
 top of the existing code.

From my point of view, Anders looks for something that works, so 
following the rules that the minimal set of changes minimize the chances
of introducing new bugs while cleaning, I would go for the minimal set 
of changes, such as:

diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..8150510 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -165,28 +165,27 @@ struct xnsched_class {
 #endif /* CONFIG_SMP */
 
 /* Test all resched flags from the given scheduler mask. */
-static inline int xnsched_resched_p(struct xnsched *sched)
+static inline int xnsched_remote_resched_p(struct xnsched *sched)
 {
-   return testbits(sched-status, XNRESCHED);
+   return !xnarch_cpus_empty(current_sched-resched);
 }
 
-static inline int xnsched_self_resched_p(struct xnsched *sched)
+static inline int xnsched_resched_p(struct xnsched *sched)
 {
return testbits(sched-status, XNRESCHED);
 }
 
 /* Set self resched flag for the given scheduler. */
 #define xnsched_set_self_resched(__sched__) do {   \
-  setbits((__sched__)-status, XNRESCHED); \
+  __setbits((__sched__)-status, XNRESCHED);   \
 } while (0)
 
 /* Set specific resched flag into the local scheduler mask. */
 #define xnsched_set_resched(__sched__) do {\
   xnsched_t *current_sched = xnpod_current_sched();\
-  setbits(current_sched-status, XNRESCHED);   \
+  __setbits(current_sched-status, XNRESCHED); \
   if (current_sched != (__sched__)){   \
   xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);  \
-  setbits((__sched__)-status, XNRESCHED); \
   }\
 } while (0)
 
diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
index 862838c..4cb707a 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -276,18 +276,16 @@ EXPORT_SYMBOL_GPL(xnpod_fatal_helper);
 
 void xnpod_schedule_handler(void) /* Called with hw interrupts off. */
 {
-   xnsched_t *sched;
+   xnsched_t *sched = xnpod_current_sched();
 
trace_mark(xn_nucleus, sched_remote, MARK_NOARGS);
 #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
-   sched = xnpod_current_sched();
if (testbits(sched-status, XNRPICK)) {
clrbits(sched-status, XNRPICK);
xnshadow_rpi_check();
}
-#else
-   (void)sched;
 #endif /* CONFIG_SMP  CONFIG_XENO_OPT_PRIOCPL */
+   xnsched_set_self_resched(sched);
xnpod_schedule();
 }
 
@@ -2174,7 +2172,7 @@ static inline int __xnpod_test_resched(struct xnsched 
*sched)
int resched = testbits(sched-status, XNRESCHED);
 #ifdef CONFIG_SMP
/* Send resched IPI to remote CPU(s). */
-   if (unlikely(xnsched_resched_p(sched))) {
+   if (unlikely(xnsched_remote_resched_p(sched))) {
xnarch_send_ipi(sched-resched);
xnarch_cpus_clear(sched-resched);
}
diff --git a/ksrc/nucleus/timer.c b/ksrc/nucleus/timer.c
index 1fe3331..a0ac627 100644
--- a/ksrc/nucleus/timer.c
+++ b/ksrc/nucleus/timer.c
@@ -97,7 +97,7 @@ void xntimer_next_local_shot(xnsched_t *sched)
__clrbits(sched-status, XNHDEFER);
timer = aplink2timer(h);
if (unlikely(timer == sched-htimer)) {
-   if (xnsched_self_resched_p(sched) ||
+   if

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 05.11.2010 00:46, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 05.11.2010 00:25, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.
 That is not enough.
 It is, I've reviewed the code today.
 The fallouts I am talking about are:
 47dac49c71e89b684203e854d1b0172ecacbc555

 Not related.

 38f2ca83a8e63cc94eaa911ff1c0940c884b5078

 An optimization.

 5e7cfa5c25672e4478a721eadbd6f6c5b4f88a2f

 That fall out of that commit is fixed in my series.

 This commit was followed by several others to fix
 the fix. You know how things are, someone proposes a fix, which fixes
 things for him, but it breaks in the other people configurations (one of
 the fallouts was a complete revamp of include/asm-arm/atomic.h for
 instance).

 I've pushed a series that reverts that commit, then fixes and cleans up
 on top of it. Just pushed if you want to take a look. We can find some
 alternative debugging mechanism independently (though I'm curious to see
 it - it still makes no sense to me).
 Since the fix is simply a modification to what we have currently. I
 would prefer if we did not remove it. In fact, I think it would be
 simpler if we started from what we currently have than reverting past
 patches.

 Look at the series, it goes step by step to an IMHO clean state. We can
 pull out the debugging check removal, though, if you prefer to work on
 top of the existing code.
 
 From my point of view, Anders looks for something that works, so 
 following the rules that the minimal set of changes minimize the chances
 of introducing new bugs while cleaning, I would go for the minimal set 
 of changes, such as:

I don't mind where to start, you need to understand the code anyway to
asses any change, may it be a complete rewrite, revert and then rework,
or a combination like below.

 
 diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
 index df56417..8150510 100644
 --- a/include/nucleus/sched.h
 +++ b/include/nucleus/sched.h
 @@ -165,28 +165,27 @@ struct xnsched_class {
  #endif /* CONFIG_SMP */
  
  /* Test all resched flags from the given scheduler mask. */
 -static inline int xnsched_resched_p(struct xnsched *sched)
 +static inline int xnsched_remote_resched_p(struct xnsched *sched)
  {
 - return testbits(sched-status, XNRESCHED);
 + return !xnarch_cpus_empty(current_sched-resched);
  }
  
 -static inline int xnsched_self_resched_p(struct xnsched *sched)
 +static inline int xnsched_resched_p(struct xnsched *sched)
  {
   return testbits(sched-status, XNRESCHED);
  }
  
  /* Set self resched flag for the given scheduler. */
  #define xnsched_set_self_resched(__sched__) do { \
 -  setbits((__sched__)-status, XNRESCHED);   \
 +  __setbits((__sched__)-status, XNRESCHED); \
  } while (0)
  
  /* Set specific resched flag into the local scheduler mask. */
  #define xnsched_set_resched(__sched__) do {  \
xnsched_t *current_sched = xnpod_current_sched();  \
 -  setbits(current_sched-status, XNRESCHED); \
 +  __setbits(current_sched-status, XNRESCHED);   
 \
if (current_sched != (__sched__))  {   \
xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);
 \
 -  setbits((__sched__)-status, XNRESCHED);   
 \
}  \
  } while (0)
  
 diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
 index 862838c..4cb707a 100644
 --- a/ksrc/nucleus/pod.c
 +++ b/ksrc/nucleus/pod.c
 @@ -276,18 +276,16 @@ EXPORT_SYMBOL_GPL(xnpod_fatal_helper);
  
  void xnpod_schedule_handler(void) /* Called with hw interrupts off. */
  {
 - xnsched_t *sched;
 + xnsched_t *sched = xnpod_current_sched();
  
   trace_mark(xn_nucleus, sched_remote, MARK_NOARGS);
  #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
 - sched = xnpod_current_sched();
   if (testbits(sched-status, XNRPICK)) {
   clrbits(sched-status, XNRPICK);
   xnshadow_rpi_check();
   }
 -#else
 - (void)sched;
  #endif /* CONFIG_SMP  CONFIG_XENO_OPT_PRIOCPL */
 + xnsched_set_self_resched(sched);
   xnpod_schedule();
  }
  
 @@ -2174,7 +2172,7 @@ static inline int __xnpod_test_resched(struct xnsched 
 *sched)
   int resched = testbits(sched-status, XNRESCHED);
  #ifdef CONFIG_SMP
   /* Send resched IPI to remote CPU(s). */
 - if (unlikely(xnsched_resched_p(sched))) {
 + if (unlikely(xnsched_remote_resched_p(sched))) {
   xnarch_send_ipi(sched-resched);
   xnarch_cpus_clear(sched-resched);
   }
 diff --git a/ksrc/nucleus/timer.c b/ksrc/nucleus/timer.c
 index 1fe3331..a0ac627 100644
 --- a/ksrc/nucleus/timer.c
 +++ b/ksrc/nucleus/timer.c

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 05.11.2010 00:46, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 05.11.2010 00:25, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.
 That is not enough.
 It is, I've reviewed the code today.
 The fallouts I am talking about are:
 47dac49c71e89b684203e854d1b0172ecacbc555
 Not related.

 38f2ca83a8e63cc94eaa911ff1c0940c884b5078
 An optimization.

 5e7cfa5c25672e4478a721eadbd6f6c5b4f88a2f
 That fall out of that commit is fixed in my series.

 This commit was followed by several others to fix
 the fix. You know how things are, someone proposes a fix, which fixes
 things for him, but it breaks in the other people configurations (one of
 the fallouts was a complete revamp of include/asm-arm/atomic.h for
 instance).

 I've pushed a series that reverts that commit, then fixes and cleans up
 on top of it. Just pushed if you want to take a look. We can find some
 alternative debugging mechanism independently (though I'm curious to see
 it - it still makes no sense to me).
 Since the fix is simply a modification to what we have currently. I
 would prefer if we did not remove it. In fact, I think it would be
 simpler if we started from what we currently have than reverting past
 patches.
 Look at the series, it goes step by step to an IMHO clean state. We can
 pull out the debugging check removal, though, if you prefer to work on
 top of the existing code.
 From my point of view, Anders looks for something that works, so 
 following the rules that the minimal set of changes minimize the chances
 of introducing new bugs while cleaning, I would go for the minimal set 
 of changes, such as:
 
 I don't mind where to start, you need to understand the code anyway to
 asses any change, may it be a complete rewrite, revert and then rework,
 or a combination like below.
 
 diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
 index df56417..8150510 100644
 --- a/include/nucleus/sched.h
 +++ b/include/nucleus/sched.h
 @@ -165,28 +165,27 @@ struct xnsched_class {
  #endif /* CONFIG_SMP */
  
  /* Test all resched flags from the given scheduler mask. */
 -static inline int xnsched_resched_p(struct xnsched *sched)
 +static inline int xnsched_remote_resched_p(struct xnsched *sched)
  {
 -return testbits(sched-status, XNRESCHED);
 +return !xnarch_cpus_empty(current_sched-resched);
  }
  
 -static inline int xnsched_self_resched_p(struct xnsched *sched)
 +static inline int xnsched_resched_p(struct xnsched *sched)
  {
  return testbits(sched-status, XNRESCHED);
  }
  
  /* Set self resched flag for the given scheduler. */
  #define xnsched_set_self_resched(__sched__) do {\
 -  setbits((__sched__)-status, XNRESCHED);  \
 +  __setbits((__sched__)-status, XNRESCHED);\
  } while (0)
  
  /* Set specific resched flag into the local scheduler mask. */
  #define xnsched_set_resched(__sched__) do { \
xnsched_t *current_sched = xnpod_current_sched(); \
 -  setbits(current_sched-status, XNRESCHED);
 \
 +  __setbits(current_sched-status, XNRESCHED);  
 \
if (current_sched != (__sched__)) {   \
xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);   
 \
 -  setbits((__sched__)-status, XNRESCHED);  
 \
} \
  } while (0)
  
 diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
 index 862838c..4cb707a 100644
 --- a/ksrc/nucleus/pod.c
 +++ b/ksrc/nucleus/pod.c
 @@ -276,18 +276,16 @@ EXPORT_SYMBOL_GPL(xnpod_fatal_helper);
  
  void xnpod_schedule_handler(void) /* Called with hw interrupts off. */
  {
 -xnsched_t *sched;
 +xnsched_t *sched = xnpod_current_sched();
  
  trace_mark(xn_nucleus, sched_remote, MARK_NOARGS);
  #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
 -sched = xnpod_current_sched();
  if (testbits(sched-status, XNRPICK)) {
  clrbits(sched-status, XNRPICK);
  xnshadow_rpi_check();
  }
 -#else
 -(void)sched;
  #endif /* CONFIG_SMP  CONFIG_XENO_OPT_PRIOCPL */
 +xnsched_set_self_resched(sched);
  xnpod_schedule();
  }
  
 @@ -2174,7 +2172,7 @@ static inline int __xnpod_test_resched(struct xnsched 
 *sched)
  int resched = testbits(sched-status, XNRESCHED);
  #ifdef CONFIG_SMP
  /* Send resched IPI to remote CPU(s). */
 -if (unlikely(xnsched_resched_p(sched))) {
 +if (unlikely(xnsched_remote_resched_p(sched))) {
  xnarch_send_ipi(sched-resched);
  xnarch_cpus_clear(sched-resched);
  }
 diff --git a/ksrc/nucleus/timer.c b/ksrc/nucleus/timer.c
 index 1fe3331..a0ac627 100644
 --- a/ksrc/nucleus/timer.c
 +++

Re: [Xenomai-core] Potential problem with rt_eepro100

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 cf4..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.

-- 
Gilles.

Re: [Xenomai-core] Potential problem with rt_eepro100

Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 05.11.2010 00:25, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 23:08, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 rework. Safer for now is likely to revert 56ff4329ff, keeping nucleus
 debugging off.
 That is not enough.
 It is, I've reviewed the code today.
 The fallouts I am talking about are:
 47dac49c71e89b684203e854d1b0172ecacbc555
 Not related.

 38f2ca83a8e63cc94eaa911ff1c0940c884b5078
 An optimization.

 5e7cfa5c25672e4478a721eadbd6f6c5b4f88a2f
 That fall out of that commit is fixed in my series.

 This commit was followed by several others to fix
 the fix. You know how things are, someone proposes a fix, which fixes
 things for him, but it breaks in the other people configurations (one of
 the fallouts was a complete revamp of include/asm-arm/atomic.h for
 instance).

 I've pushed a series that reverts that commit, then fixes and cleans up
 on top of it. Just pushed if you want to take a look. We can find some
 alternative debugging mechanism independently (though I'm curious to see
 it - it still makes no sense to me).
 Since the fix is simply a modification to what we have currently. I
 would prefer if we did not remove it. In fact, I think it would be
 simpler if we started from what we currently have than reverting past
 patches.
 Look at the series, it goes step by step to an IMHO clean state. We can
 pull out the debugging check removal, though, if you prefer to work on
 top of the existing code.
 
 From my point of view, Anders looks for something that works, so 
 following the rules that the minimal set of changes minimize the chances
 of introducing new bugs while cleaning, I would go for the minimal set 
 of changes, such as:

The tested one (on SMP, and UP with and without unlocked ctx switch):

diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..cf4 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -165,28 +165,27 @@ struct xnsched_class {
 #endif /* CONFIG_SMP */
 
 /* Test all resched flags from the given scheduler mask. */
-static inline int xnsched_resched_p(struct xnsched *sched)
+static inline int xnsched_remote_resched_p(struct xnsched *sched)
 {
-   return testbits(sched-status, XNRESCHED);
+   return !xnarch_cpus_empty(sched-resched);
 }
 
-static inline int xnsched_self_resched_p(struct xnsched *sched)
+static inline int xnsched_resched_p(struct xnsched *sched)
 {
return testbits(sched-status, XNRESCHED);
 }
 
 /* Set self resched flag for the given scheduler. */
 #define xnsched_set_self_resched(__sched__) do {   \
-  setbits((__sched__)-status, XNRESCHED); \
+  __setbits((__sched__)-status, XNRESCHED);   \
 } while (0)
 
 /* Set specific resched flag into the local scheduler mask. */
 #define xnsched_set_resched(__sched__) do {\
   xnsched_t *current_sched = xnpod_current_sched();\
-  setbits(current_sched-status, XNRESCHED);   \
+  __setbits(current_sched-status, XNRESCHED); \
   if (current_sched != (__sched__)){   \
   xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);  \
-  setbits((__sched__)-status, XNRESCHED); \
   }\
 } while (0)
 
diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
index 862838c..4cb707a 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -276,18 +276,16 @@ EXPORT_SYMBOL_GPL(xnpod_fatal_helper);
 
 void xnpod_schedule_handler(void) /* Called with hw interrupts off. */
 {
-   xnsched_t *sched;
+   xnsched_t *sched = xnpod_current_sched();
 
trace_mark(xn_nucleus, sched_remote, MARK_NOARGS);
 #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
-   sched = xnpod_current_sched();
if (testbits(sched-status, XNRPICK)) {
clrbits(sched-status, XNRPICK);
xnshadow_rpi_check();
}
-#else
-   (void)sched;
 #endif /* CONFIG_SMP  CONFIG_XENO_OPT_PRIOCPL */
+   xnsched_set_self_resched(sched);
xnpod_schedule();
 }
 
@@ -2174,7 +2172,7 @@ static inline int __xnpod_test_resched(struct xnsched 
*sched)
int resched = testbits(sched-status, XNRESCHED);
 #ifdef CONFIG_SMP
/* Send resched IPI to remote CPU(s). */
-   if (unlikely(xnsched_resched_p(sched))) {
+   if (unlikely(xnsched_remote_resched_p(sched))) {
xnarch_send_ipi(sched-resched);
xnarch_cpus_clear(sched-resched);
}
diff --git a/ksrc/nucleus/timer.c b/ksrc/nucleus/timer.c
index 1fe3331..a0ac627 100644
--- a/ksrc/nucleus/timer.c
+++ b/ksrc/nucleus/timer.c
@@ -97,7 +97,7 @@ void xntimer_next_local_shot(xnsched_t *sched)
__clrbits(sched-status, XNHDEFER);
timer = aplink2timer(h);
if (unlikely(timer ==

Re: [Xenomai-core] Potential problem with rt_eepro100


Anders Blomdell wrote:

Jan Kiszka wrote:

Am 01.11.2010 17:55, Anders Blomdell wrote:

Jan Kiszka wrote:

Am 28.10.2010 11:34, Anders Blomdell wrote:

Jan Kiszka wrote:

Am 28.10.2010 09:34, Anders Blomdell wrote:

Anders Blomdell wrote:

Anders Blomdell wrote:

Hi,

I'm trying to use rt_eepro100, for sending raw ethernet packets,
but I'm
experincing occasionally weird behaviour.

Versions of things:

  linux-2.6.34.5
  xenomai-2.5.5.2
  rtnet-39f7fcf

The testprogram runs on two computers with Intel Corporation
82557/8/9/0/1 Ethernet Pro 100 (rev 08) controller, where one
computer
acts as a mirror sending back packets received from the ethernet
(only
those two computers on the network), and the other sends 
packets and

measures roundtrip time. Most packets comes back in approximately
100
us, but occasionally the reception times out (once in about 10
packets or more), but the packets gets immediately received when
reception is retried, which might indicate a race between
rt_dev_recvmsg
and interrupt, but I might miss something obvious.

Changing one of the ethernet cards to a Intel Corporation 82541PI
Gigabit Ethernet Controller (rev 05), while keeping everything 
else

constant, changes behavior somewhat; after receiving a few 10
packets, reception stops entirely (-EAGAIN is returned), while
transmission proceeds as it should (and mirror returns packets).

Any suggestions on what to try?
Since the problem disappears with 'maxcpus=1', I suspect I have a 
SMP

issue (machine is a Core2 Quad), so I'll move to xenomai-core.
(original message can be found at
http://sourceforge.net/mailarchive/message.php?msg_name=4CC82C8D.3080808%40control.lth.se 



)

Xenomai-core gurus: which is the corrrect way to debug SMP issues?
Can I run I-pipe-tracer and expect to be able save at least 150 
us of

traces for all cpus? Any hints/suggestions/insigths are welcome...

The i-pipe tracer unfortunately only saves traces for a the CPU that
triggered the freeze. To have a full pictures, you may want to try my
ftrace port I posted recently for 2.6.35.

2.6.35.7 ?


Exactly.

Finally managed to get the ftrace to work
(one possible bug: had to manually copy
include/xenomai/trace/xn_nucleus.h to
include/xenomai/trace/events/xn_nucleus.h), and it looks like it can be
very useful...

But I don't think it will give much info at the moment, since no
xenomai/ipipe interrupt activity shows up, and adding that is far above
my league :-(


You could use the function tracer, provided you are able to stop the
trace quickly enough on error.


My current theory is that the problem occurs when something like this
takes place:

  CPU-iCPU-jCPU-kCPU-l

rt_dev_sendmsg
xmit_irq
rt_dev_recvmsgrecv_irq


Can't follow. When races here, and what will go wrong then?

Thats the good question. Find attached:

1. .config (so you can check for stupid mistakes)
2. console log
3. latest version of test program
4. tail of ftrace dump

These are the xenomai tasks running when the test program is active:

CPU  PIDCLASS  PRI  TIMEOUT   TIMEBASE   STAT   NAME
  0  0  idle-1  - master R  ROOT/0
  1  0  idle-1  - master R  ROOT/1
  2  0  idle-1  - master R  ROOT/2
  3  0  idle-1  - master R  ROOT/3
  0  0  rt  98  - master W  rtnet-stack
  0  0  rt   0  - master W  rtnet-rtpc
  0  29901  rt  50  - masterraw_test
  0  29906  rt   0  - master X  reporter



The lines of interest from the trace are probably:

[003]  2061.347855: xn_nucleus_thread_resume: thread=f9bf7b00
  thread_name=rtnet-stack mask=2

[003]  2061.347862: xn_nucleus_sched: status=200
[000]  2061.347866: xn_nucleus_sched_remote: status=0

since this is the only place where a packet gets delayed, and the only 
place in the trace where sched_remote reports a status=0
Since the cpu that has rtnet-stack and hence should be resumed is doing 
heavy I/O at the time of fault; could it be that 
send_ipi/schedule_handler needs barriers to make sure taht decisions are 
made on the right status?


/Anders


___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 03.11.2010 12:44, Anders Blomdell wrote:
 Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 01.11.2010 17:55, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 28.10.2010 11:34, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 28.10.2010 09:34, Anders Blomdell wrote:
 Anders Blomdell wrote:
 Anders Blomdell wrote:
 Hi,

 I'm trying to use rt_eepro100, for sending raw ethernet packets,
 but I'm
 experincing occasionally weird behaviour.

 Versions of things:

   linux-2.6.34.5
   xenomai-2.5.5.2
   rtnet-39f7fcf

 The testprogram runs on two computers with Intel Corporation
 82557/8/9/0/1 Ethernet Pro 100 (rev 08) controller, where one
 computer
 acts as a mirror sending back packets received from the ethernet
 (only
 those two computers on the network), and the other sends
 packets and
 measures roundtrip time. Most packets comes back in approximately
 100
 us, but occasionally the reception times out (once in about
 10
 packets or more), but the packets gets immediately received when
 reception is retried, which might indicate a race between
 rt_dev_recvmsg
 and interrupt, but I might miss something obvious.
 Changing one of the ethernet cards to a Intel Corporation 82541PI
 Gigabit Ethernet Controller (rev 05), while keeping everything
 else
 constant, changes behavior somewhat; after receiving a few 10
 packets, reception stops entirely (-EAGAIN is returned), while
 transmission proceeds as it should (and mirror returns packets).

 Any suggestions on what to try?
 Since the problem disappears with 'maxcpus=1', I suspect I have
 a SMP
 issue (machine is a Core2 Quad), so I'll move to xenomai-core.
 (original message can be found at
 http://sourceforge.net/mailarchive/message.php?msg_name=4CC82C8D.3080808%40control.lth.se


 )

 Xenomai-core gurus: which is the corrrect way to debug SMP issues?
 Can I run I-pipe-tracer and expect to be able save at least 150
 us of
 traces for all cpus? Any hints/suggestions/insigths are welcome...
 The i-pipe tracer unfortunately only saves traces for a the CPU that
 triggered the freeze. To have a full pictures, you may want to
 try my
 ftrace port I posted recently for 2.6.35.
 2.6.35.7 ?

 Exactly.
 Finally managed to get the ftrace to work
 (one possible bug: had to manually copy
 include/xenomai/trace/xn_nucleus.h to
 include/xenomai/trace/events/xn_nucleus.h), and it looks like it can be
 very useful...

 But I don't think it will give much info at the moment, since no
 xenomai/ipipe interrupt activity shows up, and adding that is far above
 my league :-(

 You could use the function tracer, provided you are able to stop the
 trace quickly enough on error.

 My current theory is that the problem occurs when something like this
 takes place:

   CPU-iCPU-jCPU-kCPU-l

 rt_dev_sendmsg
 xmit_irq
 rt_dev_recvmsgrecv_irq

 Can't follow. When races here, and what will go wrong then?
 Thats the good question. Find attached:

 1. .config (so you can check for stupid mistakes)
 2. console log
 3. latest version of test program
 4. tail of ftrace dump

 These are the xenomai tasks running when the test program is active:

 CPU  PIDCLASS  PRI  TIMEOUT   TIMEBASE   STAT   NAME
   0  0  idle-1  - master R  ROOT/0
   1  0  idle-1  - master R  ROOT/1
   2  0  idle-1  - master R  ROOT/2
   3  0  idle-1  - master R  ROOT/3
   0  0  rt  98  - master W  rtnet-stack
   0  0  rt   0  - master W  rtnet-rtpc
   0  29901  rt  50  - masterraw_test
   0  29906  rt   0  - master X  reporter



 The lines of interest from the trace are probably:

 [003]  2061.347855: xn_nucleus_thread_resume: thread=f9bf7b00   
   thread_name=rtnet-stack mask=2
 [003]  2061.347862: xn_nucleus_sched: status=200
 [000]  2061.347866: xn_nucleus_sched_remote: status=0

 since this is the only place where a packet gets delayed, and the only
 place in the trace where sched_remote reports a status=0
 Since the cpu that has rtnet-stack and hence should be resumed is doing
 heavy I/O at the time of fault; could it be that
 send_ipi/schedule_handler needs barriers to make sure taht decisions are
 made on the right status?

That was my first idea as well - but we should run all relevant code
under nklock here. But please correct me if I miss something.

Jan

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

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 03.11.2010 12:50, Jan Kiszka wrote:
Am 03.11.2010 12:44, Anders Blomdell wrote:
Anders Blomdell wrote:
Jan Kiszka wrote:
Am 01.11.2010 17:55, Anders Blomdell wrote:
Jan Kiszka wrote:
Am 28.10.2010 11:34, Anders Blomdell wrote:
Jan Kiszka wrote:
Am 28.10.2010 09:34, Anders Blomdell wrote:
Anders Blomdell wrote:
Anders Blomdell wrote:
Hi,

I'm trying to use rt_eepro100, for sending raw ethernet packets,
but I'm
experincing occasionally weird behaviour.

Versions of things:

linux-2.6.34.5
xenomai-2.5.5.2
rtnet-39f7fcf

The testprogram runs on two computers with Intel Corporation
82557/8/9/0/1 Ethernet Pro 100 (rev 08) controller, where one
computer
acts as a mirror sending back packets received from the ethernet
(only
those two computers on the network), and the other sends
packets and
measures roundtrip time. Most packets comes back in approximately
100
us, but occasionally the reception times out (once in about
10
packets or more), but the packets gets immediately received when
reception is retried, which might indicate a race between
rt_dev_recvmsg
and interrupt, but I might miss something obvious.
Changing one of the ethernet cards to a Intel Corporation 82541PI
Gigabit Ethernet Controller (rev 05), while keeping everything
else
constant, changes behavior somewhat; after receiving a few 10
packets, reception stops entirely (-EAGAIN is returned), while
transmission proceeds as it should (and mirror returns packets).

Any suggestions on what to try?
Since the problem disappears with 'maxcpus=1', I suspect I have
a SMP
issue (machine is a Core2 Quad), so I'll move to xenomai-core.
(original message can be found at
http://sourceforge.net/mailarchive/message.php?msg_name=4CC82C8D.3080808%40control.lth.se

)

Xenomai-core gurus: which is the corrrect way to debug SMP issues?
Can I run I-pipe-tracer and expect to be able save at least 150
us of
traces for all cpus? Any hints/suggestions/insigths are welcome...
The i-pipe tracer unfortunately only saves traces for a the CPU that
triggered the freeze. To have a full pictures, you may want to
try my
ftrace port I posted recently for 2.6.35.
2.6.35.7 ?

Exactly.
Finally managed to get the ftrace to work
(one possible bug: had to manually copy
include/xenomai/trace/xn_nucleus.h to
include/xenomai/trace/events/xn_nucleus.h), and it looks like it can be
very useful...

But I don't think it will give much info at the moment, since no
xenomai/ipipe interrupt activity shows up, and adding that is far above
my league :-(

You could use the function tracer, provided you are able to stop the
trace quickly enough on error.

My current theory is that the problem occurs when something like this
takes place:

CPU-iCPU-jCPU-kCPU-l

rt_dev_sendmsg
xmit_irq
rt_dev_recvmsgrecv_irq

Can't follow. When races here, and what will go wrong then?
Thats the good question. Find attached:

1. .config (so you can check for stupid mistakes)
2. console log
3. latest version of test program
4. tail of ftrace dump

These are the xenomai tasks running when the test program is active:

CPU PIDCLASS PRI TIMEOUT TIMEBASE STAT NAME
0 0 idle-1 - master R ROOT/0
1 0 idle-1 - master R ROOT/1
2 0 idle-1 - master R ROOT/2
3 0 idle-1 - master R ROOT/3
0 0 rt 98 - master W rtnet-stack
0 0 rt 0 - master W rtnet-rtpc
0 29901 rt 50 - masterraw_test
0 29906 rt 0 - master X reporter

The lines of interest from the trace are probably:

[003] 2061.347855: xn_nucleus_thread_resume: thread=f9bf7b00
thread_name=rtnet-stack mask=2
[003] 2061.347862: xn_nucleus_sched: status=200
[000] 2061.347866: xn_nucleus_sched_remote: status=0

since this is the only place where a packet gets delayed, and the only
place in the trace where sched_remote reports a status=0
Since the cpu that has rtnet-stack and hence should be resumed is doing
heavy I/O at the time of fault; could it be that
send_ipi/schedule_handler needs barriers to make sure taht decisions are
made on the right status?

That was my first idea as well - but we should run all relevant code
under nklock here. But please correct me if I miss something.

Mmmh -- not everything. The inlined XNRESCHED entry test in
xnpod_schedule runs outside nklock. But doesn't releasing nklock imply a
memory write barrier? Let me meditate...

Jan

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

___
Xenomai-core mailing list
Xenomai-core@gna.org

Re: [Xenomai-core] Potential problem with rt_eepro100

On 2010-11-03 12.55, Jan Kiszka wrote:
Am 03.11.2010 12:50, Jan Kiszka wrote:
Am 03.11.2010 12:44, Anders Blomdell wrote:
Anders Blomdell wrote:
Jan Kiszka wrote:
Am 01.11.2010 17:55, Anders Blomdell wrote:
Jan Kiszka wrote:
Am 28.10.2010 11:34, Anders Blomdell wrote:
Jan Kiszka wrote:
Am 28.10.2010 09:34, Anders Blomdell wrote:
Anders Blomdell wrote:
Anders Blomdell wrote:
Hi,

I'm trying to use rt_eepro100, for sending raw ethernet packets,
but I'm
experincing occasionally weird behaviour.

Versions of things:

linux-2.6.34.5
xenomai-2.5.5.2
rtnet-39f7fcf

)

But I don't think it will give much info at the moment, since no
xenomai/ipipe interrupt activity shows up, and adding that is far above
my league :-(

You could use the function tracer, provided you are able to stop the
trace quickly enough on error.

My current theory is that the problem occurs when something like this
takes place:

CPU-iCPU-jCPU-kCPU-l

rt_dev_sendmsg
xmit_irq
rt_dev_recvmsgrecv_irq

Can't follow. When races here, and what will go wrong then?
Thats the good question. Find attached:

1. .config (so you can check for stupid mistakes)
2. console log
3. latest version of test program
4. tail of ftrace dump

These are the xenomai tasks running when the test program is active:

The lines of interest from the trace are probably:

[003] 2061.347855: xn_nucleus_thread_resume: thread=f9bf7b00
thread_name=rtnet-stack mask=2
[003] 2061.347862: xn_nucleus_sched: status=200
[000] 2061.347866: xn_nucleus_sched_remote: status=0

That was my first idea as well - but we should run all relevant code
under nklock here. But please correct me if I miss something.
Wouldn't we need a write-barrier before the send_ipi regardless of what locks we
hold, otherwise no guarantees that the memory write reaches the target cpu
before the interrupt does?

Mmmh -- not everything. The inlined XNRESCHED entry test in
xnpod_schedule runs outside nklock. But doesn't releasing nklock imply a
memory write barrier? Let me meditate...
Wouldn't

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 03.11.2010 13:07, Anders Blomdell wrote:
 On 2010-11-03 12.55, Jan Kiszka wrote:
 Am 03.11.2010 12:50, Jan Kiszka wrote:
 Am 03.11.2010 12:44, Anders Blomdell wrote:
 Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 01.11.2010 17:55, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 28.10.2010 11:34, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 28.10.2010 09:34, Anders Blomdell wrote:
 Anders Blomdell wrote:
 Anders Blomdell wrote:
 Hi,

 I'm trying to use rt_eepro100, for sending raw ethernet packets,
 but I'm
 experincing occasionally weird behaviour.

 Versions of things:

   linux-2.6.34.5
   xenomai-2.5.5.2
   rtnet-39f7fcf

 The testprogram runs on two computers with Intel Corporation
 82557/8/9/0/1 Ethernet Pro 100 (rev 08) controller, where one
 computer
 acts as a mirror sending back packets received from the ethernet
 (only
 those two computers on the network), and the other sends
 packets and
 measures roundtrip time. Most packets comes back in approximately
 100
 us, but occasionally the reception times out (once in about
 10
 packets or more), but the packets gets immediately received when
 reception is retried, which might indicate a race between
 rt_dev_recvmsg
 and interrupt, but I might miss something obvious.
 Changing one of the ethernet cards to a Intel Corporation 82541PI
 Gigabit Ethernet Controller (rev 05), while keeping everything
 else
 constant, changes behavior somewhat; after receiving a few 10
 packets, reception stops entirely (-EAGAIN is returned), while
 transmission proceeds as it should (and mirror returns packets).

 Any suggestions on what to try?
 Since the problem disappears with 'maxcpus=1', I suspect I have
 a SMP
 issue (machine is a Core2 Quad), so I'll move to xenomai-core.
 (original message can be found at
 http://sourceforge.net/mailarchive/message.php?msg_name=4CC82C8D.3080808%40control.lth.se


 )

 Xenomai-core gurus: which is the corrrect way to debug SMP issues?
 Can I run I-pipe-tracer and expect to be able save at least 150
 us of
 traces for all cpus? Any hints/suggestions/insigths are welcome...
 The i-pipe tracer unfortunately only saves traces for a the CPU that
 triggered the freeze. To have a full pictures, you may want to
 try my
 ftrace port I posted recently for 2.6.35.
 2.6.35.7 ?

 Exactly.
 Finally managed to get the ftrace to work
 (one possible bug: had to manually copy
 include/xenomai/trace/xn_nucleus.h to
 include/xenomai/trace/events/xn_nucleus.h), and it looks like it can be
 very useful...

 But I don't think it will give much info at the moment, since no
 xenomai/ipipe interrupt activity shows up, and adding that is far above
 my league :-(

 You could use the function tracer, provided you are able to stop the
 trace quickly enough on error.

 My current theory is that the problem occurs when something like this
 takes place:

   CPU-iCPU-jCPU-kCPU-l

 rt_dev_sendmsg
 xmit_irq
 rt_dev_recvmsgrecv_irq

 Can't follow. When races here, and what will go wrong then?
 Thats the good question. Find attached:

 1. .config (so you can check for stupid mistakes)
 2. console log
 3. latest version of test program
 4. tail of ftrace dump

 These are the xenomai tasks running when the test program is active:

 CPU  PIDCLASS  PRI  TIMEOUT   TIMEBASE   STAT   NAME
   0  0  idle-1  - master R  ROOT/0
   1  0  idle-1  - master R  ROOT/1
   2  0  idle-1  - master R  ROOT/2
   3  0  idle-1  - master R  ROOT/3
   0  0  rt  98  - master W  rtnet-stack
   0  0  rt   0  - master W  rtnet-rtpc
   0  29901  rt  50  - masterraw_test
   0  29906  rt   0  - master X  reporter



 The lines of interest from the trace are probably:

 [003]  2061.347855: xn_nucleus_thread_resume: thread=f9bf7b00   
   thread_name=rtnet-stack mask=2
 [003]  2061.347862: xn_nucleus_sched: status=200
 [000]  2061.347866: xn_nucleus_sched_remote: status=0

 since this is the only place where a packet gets delayed, and the only
 place in the trace where sched_remote reports a status=0
 Since the cpu that has rtnet-stack and hence should be resumed is doing
 heavy I/O at the time of fault; could it be that
 send_ipi/schedule_handler needs barriers to make sure taht decisions are
 made on the right status?

 That was my first idea as well - but we should run all relevant code
 under nklock here. But please correct me if I miss something.
 Wouldn't we need a write-barrier before the send_ipi regardless of what locks 
 we
 hold, otherwise no guarantees that the memory write reaches the target cpu
 before the interrupt does?

Yeah, the problem is that if xnpod_resume_thread and the next
xnpod_reschedule are under the same nklock, we won't issue the barrier
as we

Re: [Xenomai-core] Potential problem with rt_eepro100


Jan Kiszka wrote:

additional barrier. Can you check this?

diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..66b52ad 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct xnsched 
*sched)
   if (current_sched != (__sched__)){   \
   xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);   \
   setbits((__sched__)-status, XNRESCHED);  \
+  xnarch_memory_barrier(); \
   }\
 } while (0)


In progress, if nothing breaks before, I'll report status tomorrow morning.


Mmmh -- not everything. The inlined XNRESCHED entry test in
xnpod_schedule runs outside nklock. But doesn't releasing nklock imply a
memory write barrier? Let me meditate...

Wouldn't we need a read barrier then (but maybe the irq-handling takes care of
that, not familiar with the code yet)?


A read barrier is not required here as we do not need to order load
operation /wrt each other in the reschedule IRQ handler.

Only if taking the interrupt is equivalent to:

  read interrupts status
  memory_read_barrier
  execute handler

processor manuals should have the answer to this (or it might already be 
in the code)...



You can always help: there is a lot boring^Winteresting tracepoint
conversion waiting in Xenomai, see the few already converted nucleus
tracepoints.

As soon as I have my system running, I'll put some effort into this.

/Anders


___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100


Anders Blomdell wrote:

Jan Kiszka wrote:

additional barrier. Can you check this?

diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..66b52ad 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct 
xnsched *sched)

   if (current_sched != (__sched__)){\
   xnarch_cpu_set(xnsched_cpu(__sched__), 
current_sched-resched);\

   setbits((__sched__)-status, XNRESCHED);\
+  xnarch_memory_barrier();\
   }\
 } while (0)


In progress, if nothing breaks before, I'll report status tomorrow morning.
It still breaks (in approximately the same way). I'm currently putting a 
barrier in the other macro doing a RESCHED, also adding some tracing to 
see if a read barrier is needed.


Interesting side-note:

Harddisk accesses seems to get real slow after error has occured (kernel 
installs progresses with 2-3 modules installed per second), while lots 
of idle time reported on all cpu's, weird...


/Anders

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100


Anders Blomdell wrote:

Anders Blomdell wrote:

Jan Kiszka wrote:

additional barrier. Can you check this?

diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..66b52ad 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct 
xnsched *sched)

   if (current_sched != (__sched__)){\
   xnarch_cpu_set(xnsched_cpu(__sched__), 
current_sched-resched);\

   setbits((__sched__)-status, XNRESCHED);\
+  xnarch_memory_barrier();\
   }\
 } while (0)


In progress, if nothing breaks before, I'll report status tomorrow 
morning.
It still breaks (in approximately the same way). I'm currently putting a 
barrier in the other macro doing a RESCHED, also adding some tracing to 
see if a read barrier is needed.
Nope, no luck there either. Will start interesting tracepoint 
adding/conversion :-(


Any reason why xn_nucleus_sched_remote should ever report status = 0?

/Anders


___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 03.11.2010 17:46, Anders Blomdell wrote:
 Anders Blomdell wrote:
 Anders Blomdell wrote:
 Jan Kiszka wrote:
 additional barrier. Can you check this?

 diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
 index df56417..66b52ad 100644
 --- a/include/nucleus/sched.h
 +++ b/include/nucleus/sched.h
 @@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct 
 xnsched *sched)
if (current_sched != (__sched__)){\
xnarch_cpu_set(xnsched_cpu(__sched__), 
 current_sched-resched);\
setbits((__sched__)-status, XNRESCHED);\
 +  xnarch_memory_barrier();\
}\
  } while (0)

 In progress, if nothing breaks before, I'll report status tomorrow 
 morning.
 It still breaks (in approximately the same way). I'm currently putting a 
 barrier in the other macro doing a RESCHED, also adding some tracing to 
 see if a read barrier is needed.
 Nope, no luck there either. Will start interesting tracepoint 
 adding/conversion :-(

Strange. But it was too easy anyway...

 
 Any reason why xn_nucleus_sched_remote should ever report status = 0?

Really don't know yet. You could trigger on this state and call
ftrace_stop() then. Provided you had the functions tracer enabled, that
should give a nice pictures of what happened before.

Jan

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

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100


Jan Kiszka wrote:

Am 03.11.2010 17:46, Anders Blomdell wrote:

Anders Blomdell wrote:

Anders Blomdell wrote:

Jan Kiszka wrote:

additional barrier. Can you check this?

diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..66b52ad 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct 
xnsched *sched)

   if (current_sched != (__sched__)){\
   xnarch_cpu_set(xnsched_cpu(__sched__), 
current_sched-resched);\

   setbits((__sched__)-status, XNRESCHED);\
+  xnarch_memory_barrier();\
   }\
 } while (0)
In progress, if nothing breaks before, I'll report status tomorrow 
morning.
It still breaks (in approximately the same way). I'm currently putting a 
barrier in the other macro doing a RESCHED, also adding some tracing to 
see if a read barrier is needed.
Nope, no luck there either. Will start interesting tracepoint 
adding/conversion :-(


Strange. But it was too easy anyway...


Any reason why xn_nucleus_sched_remote should ever report status = 0?


Really don't know yet. You could trigger on this state and call
ftrace_stop() then. Provided you had the functions tracer enabled, that
should give a nice pictures of what happened before.


Isn't there a race betweeen these two (still waiting for compilation to 
be finished)?


static inline int __xnpod_test_resched(struct xnsched *sched)
{
int resched = testbits(sched-status, XNRESCHED);
#ifdef CONFIG_SMP
/* Send resched IPI to remote CPU(s). */
if (unlikely(xnsched_resched_p(sched))) {
xnarch_send_ipi(sched-resched);
xnarch_cpus_clear(sched-resched);
}
#endif
clrbits(sched-status, XNRESCHED);
return resched;
}

#define xnsched_set_resched(__sched__) do {   \
  xnsched_t *current_sched = xnpod_current_sched();   \
  setbits(current_sched-status, XNRESCHED);  \
  if (current_sched != (__sched__)) { \
  xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched); \
  setbits((__sched__)-status, XNRESCHED);\
  xnarch_memory_barrier();\
  }   \
} while (0)

I would suggest (if I have got all the macros right):

static inline int __xnpod_test_resched(struct xnsched *sched)
{
int resched = testbits(sched-status, XNRESCHED);
if (unlikely(resched)) {
#ifdef CONFIG_SMP
/* Send resched IPI to remote CPU(s). */
xnarch_send_ipi(sched-resched);
xnarch_cpus_clear(sched-resched);
#endif
clrbits(sched-status, XNRESCHED);
}
return resched;
}

/Anders


___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

2010-11-03 Thread Philippe Gerum

On Wed, 2010-11-03 at 20:38 +0100, Anders Blomdell wrote:
 Jan Kiszka wrote:
  Am 03.11.2010 17:46, Anders Blomdell wrote:
  Anders Blomdell wrote:
  Anders Blomdell wrote:
  Jan Kiszka wrote:
  additional barrier. Can you check this?
 
  diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
  index df56417..66b52ad 100644
  --- a/include/nucleus/sched.h
  +++ b/include/nucleus/sched.h
  @@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct 
  xnsched *sched)
 if (current_sched != (__sched__)){\
 xnarch_cpu_set(xnsched_cpu(__sched__), 
  current_sched-resched);\
 setbits((__sched__)-status, XNRESCHED);\
  +  xnarch_memory_barrier();\
 }\
   } while (0)
  In progress, if nothing breaks before, I'll report status tomorrow 
  morning.
  It still breaks (in approximately the same way). I'm currently putting a 
  barrier in the other macro doing a RESCHED, also adding some tracing to 
  see if a read barrier is needed.
  Nope, no luck there either. Will start interesting tracepoint 
  adding/conversion :-(
  
  Strange. But it was too easy anyway...
  
  Any reason why xn_nucleus_sched_remote should ever report status = 0?
  
  Really don't know yet. You could trigger on this state and call
  ftrace_stop() then. Provided you had the functions tracer enabled, that
  should give a nice pictures of what happened before.
 
 Isn't there a race betweeen these two (still waiting for compilation to 
 be finished)?

We always hold the nklock in both contexts.

 
 static inline int __xnpod_test_resched(struct xnsched *sched)
 {
   int resched = testbits(sched-status, XNRESCHED);
 #ifdef CONFIG_SMP
   /* Send resched IPI to remote CPU(s). */
   if (unlikely(xnsched_resched_p(sched))) {
   xnarch_send_ipi(sched-resched);
   xnarch_cpus_clear(sched-resched);
   }
 #endif
   clrbits(sched-status, XNRESCHED);
   return resched;
 }
 
 #define xnsched_set_resched(__sched__) do {   \
xnsched_t *current_sched = xnpod_current_sched();   \
setbits(current_sched-status, XNRESCHED);  \
if (current_sched != (__sched__)) { \
xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched); \
setbits((__sched__)-status, XNRESCHED);\
xnarch_memory_barrier();\
}   \
 } while (0)
 
 I would suggest (if I have got all the macros right):
 
 static inline int __xnpod_test_resched(struct xnsched *sched)
 {
   int resched = testbits(sched-status, XNRESCHED);
   if (unlikely(resched)) {
 #ifdef CONFIG_SMP
   /* Send resched IPI to remote CPU(s). */
   xnarch_send_ipi(sched-resched);
   xnarch_cpus_clear(sched-resched);
 #endif
   clrbits(sched-status, XNRESCHED);
   }
   return resched;
 }
 
 /Anders
 
 
 ___
 Xenomai-core mailing list
 Xenomai-core@gna.org
 https://mail.gna.org/listinfo/xenomai-core

-- 
Philippe.



___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 03.11.2010 21:41, Philippe Gerum wrote:
 On Wed, 2010-11-03 at 20:38 +0100, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 17:46, Anders Blomdell wrote:
 Anders Blomdell wrote:
 Anders Blomdell wrote:
 Jan Kiszka wrote:
 additional barrier. Can you check this?

 diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
 index df56417..66b52ad 100644
 --- a/include/nucleus/sched.h
 +++ b/include/nucleus/sched.h
 @@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct 
 xnsched *sched)
if (current_sched != (__sched__)){\
xnarch_cpu_set(xnsched_cpu(__sched__), 
 current_sched-resched);\
setbits((__sched__)-status, XNRESCHED);\
 +  xnarch_memory_barrier();\
}\
  } while (0)
 In progress, if nothing breaks before, I'll report status tomorrow 
 morning.
 It still breaks (in approximately the same way). I'm currently putting a 
 barrier in the other macro doing a RESCHED, also adding some tracing to 
 see if a read barrier is needed.
 Nope, no luck there either. Will start interesting tracepoint 
 adding/conversion :-(

 Strange. But it was too easy anyway...

 Any reason why xn_nucleus_sched_remote should ever report status = 0?

 Really don't know yet. You could trigger on this state and call
 ftrace_stop() then. Provided you had the functions tracer enabled, that
 should give a nice pictures of what happened before.

 Isn't there a race betweeen these two (still waiting for compilation to 
 be finished)?
 
 We always hold the nklock in both contexts.
 

But we not not always use atomic ops for manipulating status bits (but
we do in other cases where this is no need - different story). This may
fix the race:

diff --git a/ksrc/nucleus/intr.c b/ksrc/nucleus/intr.c
index d7a772f..af8ebeb 100644
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -85,7 +85,7 @@ static void xnintr_irq_handler(unsigned irq, void *cookie);
 
 void xnintr_host_tick(struct xnsched *sched) /* Interrupts off. */
 {
-   __clrbits(sched-status, XNHTICK);
+   clrbits(sched-status, XNHTICK);
xnarch_relay_tick();
 }
 
@@ -105,11 +105,13 @@ void xnintr_clock_handler(void)
trace_mark(xn_nucleus, irq_enter, irq %u, XNARCH_TIMER_IRQ);
trace_mark(xn_nucleus, tbase_tick, base %s, nktbase.name);
 
+   xnlock_get(nklock);
+
++sched-inesting;
__setbits(sched-status, XNINIRQ);
 
-   xnlock_get(nklock);
xntimer_tick_aperiodic();
+
xnlock_put(nklock);
 
xnstat_counter_inc(nkclock.stat[xnsched_cpu(sched)].hits);
@@ -117,7 +119,7 @@ void xnintr_clock_handler(void)
nkclock.stat[xnsched_cpu(sched)].account, start);
 
if (--sched-inesting == 0) {
-   __clrbits(sched-status, XNINIRQ);
+   clrbits(sched-status, XNINIRQ);
xnpod_schedule();
}
/*
@@ -178,7 +180,7 @@ static void xnintr_shirq_handler(unsigned irq, void *cookie)
trace_mark(xn_nucleus, irq_enter, irq %u, irq);
 
++sched-inesting;
-   __setbits(sched-status, XNINIRQ);
+   setbits(sched-status, XNINIRQ);
 
xnlock_get(shirq-lock);
intr = shirq-handlers;
@@ -220,7 +222,7 @@ static void xnintr_shirq_handler(unsigned irq, void *cookie)
xnarch_end_irq(irq);
 
if (--sched-inesting == 0) {
-   __clrbits(sched-status, XNINIRQ);
+   clrbits(sched-status, XNINIRQ);
xnpod_schedule();
}
 
@@ -247,7 +249,7 @@ static void xnintr_edge_shirq_handler(unsigned irq, void 
*cookie)
trace_mark(xn_nucleus, irq_enter, irq %u, irq);
 
++sched-inesting;
-   __setbits(sched-status, XNINIRQ);
+   setbits(sched-status, XNINIRQ);
 
xnlock_get(shirq-lock);
intr = shirq-handlers;
@@ -303,7 +305,7 @@ static void xnintr_edge_shirq_handler(unsigned irq, void 
*cookie)
xnarch_end_irq(irq);
 
if (--sched-inesting == 0) {
-   __clrbits(sched-status, XNINIRQ);
+   clrbits(sched-status, XNINIRQ);
xnpod_schedule();
}
trace_mark(xn_nucleus, irq_exit, irq %u, irq);
@@ -446,7 +448,7 @@ static void xnintr_irq_handler(unsigned irq, void *cookie)
trace_mark(xn_nucleus, irq_enter, irq %u, irq);
 
++sched-inesting;
-   __setbits(sched-status, XNINIRQ);
+   setbits(sched-status, XNINIRQ);
 
xnlock_get(xnirqs[irq].lock);
 
@@ -493,7 +495,7 @@ static void xnintr_irq_handler(unsigned irq, void *cookie)
xnarch_end_irq(irq);
 
if (--sched-inesting == 0) {
-   __clrbits(sched-status, XNINIRQ);
+   clrbits(sched-status, XNINIRQ);
xnpod_schedule();
}
 

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 03.11.2010 23:03, Jan Kiszka wrote:
 Am 03.11.2010 21:41, Philippe Gerum wrote:
 On Wed, 2010-11-03 at 20:38 +0100, Anders Blomdell wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 17:46, Anders Blomdell wrote:
 Anders Blomdell wrote:
 Anders Blomdell wrote:
 Jan Kiszka wrote:
 additional barrier. Can you check this?

 diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
 index df56417..66b52ad 100644
 --- a/include/nucleus/sched.h
 +++ b/include/nucleus/sched.h
 @@ -187,6 +187,7 @@ static inline int xnsched_self_resched_p(struct 
 xnsched *sched)
if (current_sched != (__sched__)){\
xnarch_cpu_set(xnsched_cpu(__sched__), 
 current_sched-resched);\
setbits((__sched__)-status, XNRESCHED);\
 +  xnarch_memory_barrier();\
}\
  } while (0)
 In progress, if nothing breaks before, I'll report status tomorrow 
 morning.
 It still breaks (in approximately the same way). I'm currently putting a 
 barrier in the other macro doing a RESCHED, also adding some tracing to 
 see if a read barrier is needed.
 Nope, no luck there either. Will start interesting tracepoint 
 adding/conversion :-(

 Strange. But it was too easy anyway...

 Any reason why xn_nucleus_sched_remote should ever report status = 0?

 Really don't know yet. You could trigger on this state and call
 ftrace_stop() then. Provided you had the functions tracer enabled, that
 should give a nice pictures of what happened before.

 Isn't there a race betweeen these two (still waiting for compilation to 
 be finished)?

 We always hold the nklock in both contexts.

 
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:

Err, nonsense. As we manipulate xnsched::status also outside of nklock
protection, we must _always_ use atomic ops.

This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
should be pushed in a separate status word that can then be safely
modified non-atomically.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:
 
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.
 
 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.

Second try to fix and clean up the sched status bits. Anders, please
test.

Jan

diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
index 01ff0a7..5987a1f 100644
--- a/include/nucleus/pod.h
+++ b/include/nucleus/pod.h
@@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
 * context is active, or if we are caught in the middle of a
 * unlocked context switch.
 */
-#if XENO_DEBUG(NUCLEUS)
if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
return;
-#else /* !XENO_DEBUG(NUCLEUS) */
-   if (testbits(sched-status,
-XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
+#if !XENO_DEBUG(NUCLEUS)
+   if (!sched-resched)
return;
 #endif /* !XENO_DEBUG(NUCLEUS) */
 
diff --git a/include/nucleus/sched.h b/include/nucleus/sched.h
index df56417..1850208 100644
--- a/include/nucleus/sched.h
+++ b/include/nucleus/sched.h
@@ -44,7 +44,6 @@
 #define XNINTCK0x1000  /* In master tick handler 
context */
 #define XNINIRQ0x0800  /* In IRQ handling context */
 #define XNSWLOCK   0x0400  /* In context switch */
-#define XNRESCHED  0x0200  /* Needs rescheduling */
 #define XNHDEFER   0x0100  /* Host tick deferred */
 
 struct xnsched_rt {
@@ -63,7 +62,8 @@ typedef struct xnsched {
xnflags_t status;   /*! Scheduler specific status bitmask. 
*/
int cpu;
struct xnthread *curr;  /*! Current thread. */
-   xnarch_cpumask_t resched;   /*! Mask of CPUs needing rescheduling. 
*/
+   xnarch_cpumask_t remote_resched; /*! Mask of CPUs needing 
rescheduling. */
+   int resched;/*! Rescheduling needed. */
 
struct xnsched_rt rt;   /*! Context of built-in real-time 
class. */
 #ifdef CONFIG_XENO_OPT_SCHED_TP
@@ -164,30 +164,21 @@ struct xnsched_class {
 #define xnsched_cpu(__sched__) ({ (void)__sched__; 0; })
 #endif /* CONFIG_SMP */
 
-/* Test all resched flags from the given scheduler mask. */
-static inline int xnsched_resched_p(struct xnsched *sched)
-{
-   return testbits(sched-status, XNRESCHED);
-}
-
-static inline int xnsched_self_resched_p(struct xnsched *sched)
-{
-   return testbits(sched-status, XNRESCHED);
-}
-
 /* Set self resched flag for the given scheduler. */
 #define xnsched_set_self_resched(__sched__) do {   \
-  setbits((__sched__)-status, XNRESCHED); \
+   (__sched__)-resched = 1;   \
 } while (0)
 
 /* Set specific resched flag into the local scheduler mask. */
 #define xnsched_set_resched(__sched__) do {\
-  xnsched_t *current_sched = xnpod_current_sched();\
-  setbits(current_sched-status, XNRESCHED);   \
-  if (current_sched != (__sched__)){   \
-  xnarch_cpu_set(xnsched_cpu(__sched__), current_sched-resched);  \
-  setbits((__sched__)-status, XNRESCHED); \
-  }\
+   xnsched_t *current_sched = xnpod_current_sched();   \
+   current_sched-resched = 1; \
+   if (current_sched != (__sched__)) { \
+   xnarch_cpu_set(xnsched_cpu(__sched__),  \
+  current_sched-remote_resched);  \
+   (__sched__)-resched = 1;   \
+   xnarch_memory_barrier();\
+   }   \
 } while (0)
 
 void xnsched_zombie_hooks(struct xnthread *thread);
@@ -209,7 +200,7 @@ struct xnsched *xnsched_finish_unlocked_switch(struct 
xnsched *sched);
 static inline
 int xnsched_maybe_resched_after_unlocked_switch(struct xnsched *sched)
 {
-   return testbits(sched-status, XNRESCHED);
+   return sched-resched;
 }
 
 #else /* !CONFIG_XENO_HW_UNLOCKED_SWITCH */
diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
index 9e135f3..f7f8b2c 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -284,7 +284,7 @@ void xnpod_schedule_handler(void) /* Called with hw 
interrupts off. */
trace_xn_nucleus_sched_remote(sched);
 #if defined(CONFIG_SMP)  defined(CONFIG_XENO_OPT_PRIOCPL)
if

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 
 Second try to fix and clean up the sched status bits. Anders, please
 test.
 
 Jan
 
 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
* context is active, or if we are caught in the middle of a
* unlocked context switch.
*/
 -#if XENO_DEBUG(NUCLEUS)
   if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
   return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 - if (testbits(sched-status,
 -  XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 + if (!sched-resched)
   return;
  #endif /* !XENO_DEBUG(NUCLEUS) */

Having only one test was really nice here, maybe we simply read a
barrier before reading the status?

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.

 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
   * context is active, or if we are caught in the middle of a
   * unlocked context switch.
   */
 -#if XENO_DEBUG(NUCLEUS)
  if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
  return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -if (testbits(sched-status,
 - XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +if (!sched-resched)
  return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?
 

I agree - but the alternative is letting all modifications of
xnsched::status use atomic bitops (that's required when folding all bits
into a single word). And that should be much more costly, specifically
on SMP.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
  * context is active, or if we are caught in the middle of a
  * unlocked context switch.
  */
 -#if XENO_DEBUG(NUCLEUS)
 if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
 return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -   if (testbits(sched-status,
 -XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +   if (!sched-resched)
 return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?

 
 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when folding all bits
 into a single word). And that should be much more costly, specifically
 on SMP.

What about issuing a barrier before testing the status?

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
 * context is active, or if we are caught in the middle of a
 * unlocked context switch.
 */
 -#if XENO_DEBUG(NUCLEUS)
if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -  if (testbits(sched-status,
 -   XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +  if (!sched-resched)
return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?


 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when folding all bits
 into a single word). And that should be much more costly, specifically
 on SMP.
 
 What about issuing a barrier before testing the status?
 

The problem is not about reading but writing the status concurrently,
thus it's not about the code you see above.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
* context is active, or if we are caught in the middle of a
* unlocked context switch.
*/
 -#if XENO_DEBUG(NUCLEUS)
   if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
   return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 - if (testbits(sched-status,
 -  XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 + if (!sched-resched)
   return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?

 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when folding all bits
 into a single word). And that should be much more costly, specifically
 on SMP.
 What about issuing a barrier before testing the status?

 
 The problem is not about reading but writing the status concurrently,
 thus it's not about the code you see above.

The bits are modified under nklock, which implies a barrier when
unlocked. Furthermore, an IPI is guaranteed to be received on the remote
CPU after this barrier, so, a barrier should be enough to see the
modifications which have been made remotely.


-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 00:44, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
   * context is active, or if we are caught in the middle of a
   * unlocked context switch.
   */
 -#if XENO_DEBUG(NUCLEUS)
  if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
  return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -if (testbits(sched-status,
 - XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +if (!sched-resched)
  return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?

 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when folding all bits
 into a single word). And that should be much more costly, specifically
 on SMP.
 What about issuing a barrier before testing the status?


 The problem is not about reading but writing the status concurrently,
 thus it's not about the code you see above.
 
 The bits are modified under nklock, which implies a barrier when
 unlocked. Furthermore, an IPI is guaranteed to be received on the remote
 CPU after this barrier, so, a barrier should be enough to see the
 modifications which have been made remotely.

Check nucleus/intr.c for tons of unprotected status modifications.

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Jan Kiszka wrote:
 Am 04.11.2010 00:44, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits (but
 we do in other cases where this is no need - different story). This 
 may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
  * context is active, or if we are caught in the middle of a
  * unlocked context switch.
  */
 -#if XENO_DEBUG(NUCLEUS)
 if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
 return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -   if (testbits(sched-status,
 -XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +   if (!sched-resched)
 return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?

 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when folding all bits
 into a single word). And that should be much more costly, specifically
 on SMP.
 What about issuing a barrier before testing the status?

 The problem is not about reading but writing the status concurrently,
 thus it's not about the code you see above.
 The bits are modified under nklock, which implies a barrier when
 unlocked. Furthermore, an IPI is guaranteed to be received on the remote
 CPU after this barrier, so, a barrier should be enough to see the
 modifications which have been made remotely.
 
 Check nucleus/intr.c for tons of unprotected status modifications.

Ok. Then maybe, we should reconsider the original decision to start
fiddling with the XNRESCHED bit remotely.

-- 
Gilles.

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100

Am 04.11.2010 00:56, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:44, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:18, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 04.11.2010 00:11, Gilles Chanteperdrix wrote:
 Jan Kiszka wrote:
 Am 03.11.2010 23:11, Jan Kiszka wrote:
 Am 03.11.2010 23:03, Jan Kiszka wrote:
 But we not not always use atomic ops for manipulating status bits 
 (but
 we do in other cases where this is no need - different story). This 
 may
 fix the race:
 Err, nonsense. As we manipulate xnsched::status also outside of nklock
 protection, we must _always_ use atomic ops.

 This screams for a cleanup: local-only bits like XNHTICK or XNINIRQ
 should be pushed in a separate status word that can then be safely
 modified non-atomically.
 Second try to fix and clean up the sched status bits. Anders, please
 test.

 Jan

 diff --git a/include/nucleus/pod.h b/include/nucleus/pod.h
 index 01ff0a7..5987a1f 100644
 --- a/include/nucleus/pod.h
 +++ b/include/nucleus/pod.h
 @@ -277,12 +277,10 @@ static inline void xnpod_schedule(void)
 * context is active, or if we are caught in the middle of a
 * unlocked context switch.
 */
 -#if XENO_DEBUG(NUCLEUS)
if (testbits(sched-status, XNKCOUT|XNINIRQ|XNSWLOCK))
return;
 -#else /* !XENO_DEBUG(NUCLEUS) */
 -  if (testbits(sched-status,
 -   XNKCOUT|XNINIRQ|XNSWLOCK|XNRESCHED) != XNRESCHED)
 +#if !XENO_DEBUG(NUCLEUS)
 +  if (!sched-resched)
return;
  #endif /* !XENO_DEBUG(NUCLEUS) */
 Having only one test was really nice here, maybe we simply read a
 barrier before reading the status?

 I agree - but the alternative is letting all modifications of
 xnsched::status use atomic bitops (that's required when folding all bits
 into a single word). And that should be much more costly, specifically
 on SMP.
 What about issuing a barrier before testing the status?

 The problem is not about reading but writing the status concurrently,
 thus it's not about the code you see above.
 The bits are modified under nklock, which implies a barrier when
 unlocked. Furthermore, an IPI is guaranteed to be received on the remote
 CPU after this barrier, so, a barrier should be enough to see the
 modifications which have been made remotely.

 Check nucleus/intr.c for tons of unprotected status modifications.
 
 Ok. Then maybe, we should reconsider the original decision to start
 fiddling with the XNRESCHED bit remotely.

...which removed complexity and fixed a race? Let's better review the
checks done in xnpod_schedule vs. its callers, I bet there is more to
save (IOW: remove the need to test for sched-resched).

Jan



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Potential problem with rt_eepro100