[Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)

[Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)

[Matthias Schneider]

Hi all,

it seems __threadobj_unlock_sched has a slight bug
and does not reset the current->policy value.
This leads to the old policy being restored, but
no longer being available for the next
__threadobj_lock_sched / __threadobj_unlock_sched
call pair.

The attached patch fixes it.

Regards,
Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unlock_sched_fix.patch
Type: text/x-patch
Size: 512 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140501/95ff33ab/attachment.bin>

Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)

[Philippe Gerum]

On 05/01/2014 01:42 PM, Matthias Schneider wrote:
> Hi all,
>
> it seems __threadobj_unlock_sched has a slight bug
> and does not reset the current->policy value.
> This leads to the old policy being restored, but
> no longer being available for the next
> __threadobj_lock_sched / __threadobj_unlock_sched
> call pair.
>

That would mean that the client code switches between different 
scheduling policies while holding the scheduler lock?

-- 
Philippe.

Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)

[Matthias Schneider]

----- Original Message -----

> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Thursday, May 1, 2014 2:54 PM
> Subject: Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)
> 
> On 05/01/2014 01:42 PM, Matthias Schneider wrote:
> 
>>  Hi all,
>> 
>>  it seems __threadobj_unlock_sched has a slight bug
>>  and does not reset the current->policy value.
>>  This leads to the old policy being restored, but
>>  no longer being available for the next
>>  __threadobj_lock_sched / __threadobj_unlock_sched
>>  call pair.
>> 
> 
> That would mean that the client code switches between different 
> scheduling policies while holding the scheduler lock?
> 
> -- 
> Philippe.
> 

Consider the following scenario:

a) threads may hold SCHED_RT, SCHED_RR or SCHED_OTHER while not in 
threadobj_lock_sched(). (see e.g. threadobj_set_priority).

b) threadobj_lock_sched() saves prio and policy and sets it to 
SCHED_RT/threadobj_lock_prio

c) threadobj_unlock_sched() will restore (correctly) previous 
settings, but not update the threadobj's policy

d) next time threadobj_lock_sched() is called, it will save prio 
and priority, howver policy will always be SCHED_RT (which is 
not the currently set policy before upgrading it)

e) threadobj_unlock_sched() will restore prio and incorrectly saved
policy, which is always SCHED_RT -> threads that originally had
SCHED_RR or SCHED_OTHER now have SCHED_RT


Changing prio while holding the scheduler lock should be handled 
fine due to :

threadobj_set_priority():
if (thobj->schedlock_depth > 0) {
  thobj->core.prio_unlocked = prio;
  thobj->core.policy_unlocked = prio ? SCHED_RT : SCHED_OTHER;
  threadobj_unlock(thobj);
  return 0;
 }

Matthias

Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)

[Philippe Gerum]

On 05/01/2014 04:23 PM, Matthias Schneider wrote:

> b) threadobj_lock_sched() saves prio and policy and sets it to
> SCHED_RT/threadobj_lock_prio

Forcing SCHED_RT in threadobj_lock() is the core issue. Doing so allows 
SCHED_OTHER callers to hold that lock: does it make sense? Maybe it's 
handy so that client code does not have to care, on the other hand, this 
very much sounds like introducing priority inversion by design.

What would be the use case for holding the sched lock over SCHED_OTHER?

-- 
Philippe.

Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)

[Matthias Schneider]

----- Original Message -----

> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Friday, May 2, 2014 10:27 AM
> Subject: Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)
> 
> On 05/01/2014 04:23 PM, Matthias Schneider wrote:
> 
>>  b) threadobj_lock_sched() saves prio and policy and sets it to
>>  SCHED_RT/threadobj_lock_prio
> 
> Forcing SCHED_RT in threadobj_lock() is the core issue. Doing so allows 
> SCHED_OTHER callers to hold that lock: does it make sense? Maybe it's 
> handy so that client code does not have to care, on the other hand, this 
> very much sounds like introducing priority inversion by design.
> 
> What would be the use case for holding the sched lock over SCHED_OTHER?
> 
> 
> -- 
> Philippe.
> 

There are some points I would like to raise about this issue:

a) the policy active before holding the "scheduler lock" is actually reapplied, 
but the threadobj info about this is left inconsistent
b) independent from wether SCHED_OTHER is a use case, SCHED_RR and SCHED_RT/SCHED_FIFO 
definitely are.
c) In my recently published FreeRTOS prototype, the "main" thread calling 
TaskSchedulerStart is actually a SCHED_OTHER thread that takes the scheduler 
lock

Nevertheless, please have a look at the code again, it seems like a simple bug 
to me.

Consider the following:

current task is SCHED_RR, prio 5

threadobj_lock_sched()
 -> SCHED_RT, prio 99

threadobj_unlock_sched()
 -> SCHED_RR, prio 5

threadobj_lock_sched()
 -> SCHED_RT, prio 99

threadobj_unlock_sched()
 -> SCHED_RT(!), prio 5

Regards,
Matthias

Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)

[Philippe Gerum]

On 05/17/2014 04:20 PM, Matthias Schneider wrote:
> ----- Original Message -----
>
>> From: Philippe Gerum <rpm@xenomai.org>
>> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
>> Cc:
>> Sent: Friday, May 2, 2014 10:27 AM
>> Subject: Re: [Xenomai] Issue in __threadobj_unlock_sched (forge/mercury)
>>
>> On 05/01/2014 04:23 PM, Matthias Schneider wrote:
>>
>>>   b) threadobj_lock_sched() saves prio and policy and sets it to
>>>   SCHED_RT/threadobj_lock_prio
>>
>> Forcing SCHED_RT in threadobj_lock() is the core issue. Doing so allows
>> SCHED_OTHER callers to hold that lock: does it make sense? Maybe it's
>> handy so that client code does not have to care, on the other hand, this
>> very much sounds like introducing priority inversion by design.
>>
>> What would be the use case for holding the sched lock over SCHED_OTHER?
>>
>>
>> --
>> Philippe.
>>
>
> There are some points I would like to raise about this issue:
>
> a) the policy active before holding the "scheduler lock" is actually reapplied,
> but the threadobj info about this is left inconsistent
> b) independent from wether SCHED_OTHER is a use case, SCHED_RR and SCHED_RT/SCHED_FIFO
> definitely are.
> c) In my recently published FreeRTOS prototype, the "main" thread calling
> TaskSchedulerStart is actually a SCHED_OTHER thread that takes the scheduler
> lock
>
> Nevertheless, please have a look at the code again, it seems like a simple bug
> to me.
>

It's on hold for now, as it could be part of a more general rework of 
the threadobj_set_priority() interface. I will definitely follow up asap 
on this though.

-- 
Philippe.