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
>> +++ 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 (xnsched_resched_p(sched) ||
>>                  !xnthread_test_state(sched->curr, XNROOT)) {
>>                      h = xntimerq_it_next(&sched->timerqueue, &it, h);
>>                      if (h) {
>>
>>
> 
> This looks correct. The next steps on top would then be
>  - avoid local reschedule if only remotes need to be signaled
>  - remaining conversions to non-atomic status access
>  - UP optimization

My point being that these changes are not essential for Anders needs,
and can be added in a second step, when we are sure that at least that
first step works.

-- 
                                                                Gilles.

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

Reply via email to