On 07/13/2011 09:04 PM, Jan Kiszka wrote:
> On 2011-07-13 20:39, Gilles Chanteperdrix wrote:
>> On 07/12/2011 07:43 PM, Jan Kiszka wrote:
>>> On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
>>>> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>>>>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>>>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>>>>                         xnlock_put_irqrestore(&nklock, s);
>>>>>>>                         xnpod_schedule();
>>>>>>>                 }
>>>>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>>>>          * to process this signal anyway.
>>>>>>>          */
>>>>>>>         if (rthal_current_domain == rthal_root_domain) {
>>>>>>> +               XENO_BUGON(NUCLEUS, xnthread_test_info(thread, 
>>>>>>> XNATOMIC));
>>>>>>
>>>>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>>>>
>>>>> Nope, I forgot to remove that line.
>>>>>
>>>>>>
>>>>>>>                 if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>>>>                     || this_task->state != TASK_RUNNING))
>>>>>>>                         xnpod_fatal
>>>>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>>>>                 return -ERESTARTSYS;
>>>>>>>         }
>>>>>>>  
>>>>>>> +       xnthread_clear_info(thread, XNATOMIC);
>>>>>>
>>>>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>>>>> place at the point it currently is.
>>>>>
>>>>> Nope. Now we either clear XNATOMIC after successful migration or when
>>>>> the signal is about to be sent (ie. in the hook). That way we can test
>>>>> more reliably (TM) in the gatekeeper if the thread can be migrated.
>>>>
>>>> Ok for adding the XNATOMIC test, because it improves the robustness, but
>>>> why changing the way XNATOMIC is set and clear? Chances of breaking
>>>> thing while changing code in this area are really high...
>>>
>>> The current code is (most probably) broken as it does not properly
>>> synchronizes the gatekeeper against a signaled and "runaway" target
>>> Linux task.
>>>
>>> We need an indication if a Linux signal will (or already has) woken up
>>> the to-be-migrated task. That task may have continued over its context,
>>> potentially on a different CPU. Providing this indication is the purpose
>>> of changing where XNATOMIC is cleared.
>>
>> What about synchronizing with the gatekeeper with a semaphore, as done
>> in the first patch you sent, but doing it in xnshadow_harden, as soon as
>> we detect that we are not back from schedule in primary mode? It seems
>> it would avoid any further issue, as we would then be guaranteed that
>> the thread could not switch to TASK_INTERRUPTIBLE again before the
>> gatekeeper is finished.
> 
> The problem is that the gatekeeper tests the task state without holding
> the task's rq lock (which is not available to us without a kernel
> patch). That cannot work reliably as long as we accept signals. That's
> why I'm trying to move state change and test under nklock.
> 
>>
>> What worries me is the comment in xnshadow_harden:
>>
>>       * gatekeeper sent us to primary mode. Since
>>       * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
>>       * the runqueue's count of uniniterruptible tasks, we just
>>       * notice the issue and gracefully fail; the caller will have
>>       * to process this signal anyway.
>>       */
>>
>> Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
>> point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
>> business of xnshadow_harden?
>>
> 
> TASK_UNINTERRUPTIBLE is not available without patching the kernel's
> scheduler for the reason mentioned in the comment (the scheduler becomes
> confused and may pick the wrong tasks, IIRC).

Does not using down/up in the taskexit event handler risk to cause the
same issue?

> 
> But I would refrain from trying to "improve" the gatekeeper design. I've
> recently mentioned this to Philippe offlist: For Xenomai 3 with some
> ipipe v3, we must rather patch schedule() to enable zero-switch domain
> migration. Means: enter the scheduler, let it suspend current and pick
> another task, but then simply escalate to the RT domain before doing any
> context switch. That's much cheaper than the current design and
> hopefully also less error-prone.

So, do you want me to merge your for-upstream branch?
-- 
                                                                Gilles.

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

Reply via email to