[PATCH] score: Simplify _Thread_Change_priority()

[Joel Sherrill]

On 5/14/2014 12:24 PM, Sebastian Huber wrote:
> On 05/14/2014 05:40 PM, Joel Sherrill wrote:
>>
>> On 5/14/2014 10:32 AM, Sebastian Huber wrote:
>>> The function to change a thread priority was too complex.  Simplify it
>>> with a new scheduler operation.  This increases the average case
>>> performance due to the simplified logic.  The interrupt disabled
>>> critical section is a bit prolonged since now the extract, update and
>>> enqueue steps are executed atomically.  This should however not impact
>>> the worst-case interrupt latency since at least for the Deterministic
>>> Priority Scheduler this sequence can be carried out with a wee bit of
>>> instructions and no loops.
>> I tend to agree with your analysis on performance. It won't have much 
>> impact
>> on the Simple since the insert still dominates. We added an extract. 
>> Big deal.
>>> Add _Scheduler_Change_priority() to replace
>>>    - _Thread_Set_transient(),
>>>    - _Scheduler_Extract(),
>>>    - _Scheduler_Enqueue(), and
>>>    - _Scheduler_Enqueue_first().
>> Do you remain "replace the sequence"?
> Ok, I will update this.
>
>>> Delete STATES_TRANSIENT, _States_Is_transient() and
>>> _Thread_Set_transient() since this state is now superfluous.
>> I am using essentially the same sequence to change affinity.
>> Set STATES_TRANSIENT, set the new affinity, remove STATES_TRANSIENT.
>>
>> Are you proposing that I change to using another scheduler operation
>> to do that?
> Which function do you implement?  Can't you use STATES_MIGRATING, like here:
As soon as I left for lunch, I realized that we already have the
set_affinity
operation and that any side-effects of changing affinity should occur there.
I was just thinking of "setting" versus "acting on the new information"
being
separate.  They really are not.
> RTEMS_INLINE_ROUTINE void _Scheduler_Set(
>    const Scheduler_Control *scheduler,
>    Thread_Control          *the_thread
> )
> {
> #if defined(RTEMS_SMP)
>    const Scheduler_Control *current_scheduler = _Scheduler_Get( 
> the_thread );
>
>    if ( current_scheduler != scheduler ) {
>      _Thread_Set_state( the_thread, STATES_MIGRATING );
>      _Scheduler_Free( _Scheduler_Get( the_thread ), the_thread );
>      the_thread->scheduler = scheduler;
>      _Scheduler_Allocate( scheduler, the_thread );
>      _Scheduler_Update( scheduler, the_thread );
>      _Thread_Clear_state( the_thread, STATES_MIGRATING );
>    }
> #else
>    (void) scheduler;
> #endif
> }
>
> Currently this function is only allowed at thread level.  Here the Giant 
> lock takes care that we don't do stupid things.
OK. I just wanted to make sure that the definition of STATES_MIGRATING
wasn't too strict to allow its use here. While changing the affinity,
there is
the issue that the thread could be running on a processor that the new
affinity does not support. Safer to simply set a state, fudge the data,
and update again.

I think the set_affinity operation will be similar to your
_Scheduler_Change_priority() code except that the
update will alter the affinity not the priority information.


>>> With this change it is possible to get rid of the
>>> SCHEDULER_SMP_NODE_IN_THE_AIR state.  This considerably simplifies the
>>> implementation of the new SMP locking protocols.
>> Agreed. I am concerned about locking while changing affinity. Is the
>> scheduler sufficiently locked that I can scan the executing list? 
> The scheduler operations are currently protected by the Giant lock and 
> ISR disabled.
>
> These transient states are very problematic if we want to introduce fine 
> grained locking some time in the future.  It can lead to an explosion of 
> states.
>
> I am not sure if you can use the existing SMP scheduler support to 
> implement arbitrary thread affinities within a scheduler instance. The 
> situation improved a bit with this patch however:
>
> http://git.rtems.org/rtems/commit/?id=38b59a6d3052654e356ae16b4a243c362312acce
>
> You may have to modify _Scheduler_SMP_Allocate_processor() and use a 
> more sophisticated _Scheduler_SMP_Is_processor_owned_by_us().
>

I think the affinity can only be honored within the processors owned by
a single schedule instance. If the thread has affinity for a processor
that is not associated with the scheduler instance, then it shouldn't
migrate schedulers.

Otherwise, in a 4 processor system with two schedulers, a thread with
a default affinity (e.g. all processors) would be pretty randomly migrating
schedulers. This doesn't seem like desirable behavior.


-- 
Joel Sherrill, Ph.D.             Director of Research & Development
joel.sherrill at OARcorp.com        On-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35805
Support Available                (256) 722-9985