<div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> @@ -257,16 +257,14 @@<br>
> #define RTEMS_SCHEDULER_STRONG_APA( name, prio_count ) \<br>
> static struct { \<br>
> Scheduler_strong_APA_Context Base; \<br>
> - Chain_Control Ready[ ( prio_count ) ]; \<br>
> + Scheduler_strong_APA_Struct Struct[ CONFIGURE_MAXIMUM_PROCESSORS ]; \<br>
<br>
I don't like this name at all either the type or the variable<br>
"Struct". Just like I wouldn't call a variable<br>
int Int;</blockquote></div></blockquote><div>Done. Changed to Scheduler_strong_APA_CPU CPU, since the structure stores all the important information corresponding to a CPU.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + *<br>
> + * The license and distribution terms for this file may be<br>
> + * found in the file LICENSE in this distribution or at<br>
> * <a href="http://www.rtems.org/license/LICENSE" rel="noreferrer" target="_blank">http://www.rtems.org/license/LICENSE</a>.<br>
relicense 2-bsd -- EB allows it, and your new code should be put under it<br></blockquote></div></blockquote><div>Done.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +#define STRONG_SCHEDULER_NODE_OF_CHAIN( node ) \<br>
> + RTEMS_CONTAINER_OF( next, Scheduler_strong_APA_Node, Chain )<br>
somehow this is not using 'node' parameter?<br></blockquote></div></blockquote><div>Silly mistake. Changed. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> - * This is an implementation of the global fixed priority scheduler (G-FP). It<br>
> - * uses one ready chain per priority to ensure constant time insert operations.<br>
> - * The scheduled chain uses linear insert operations and has at most processor<br>
> - * count entries. Since the processor and priority count are constants all<br>
> - * scheduler operations complete in a bounded execution time.<br>
> + * This is an implementation of the Strong APA scheduler defined by<br>
> + * Cerqueira et al. in Linux's Processor Affinity API, Refined:<br>
> + * Shifting Real-Time Tasks Towards Higher Schedulability.<br>
> *<br>
> - * The the_thread preempt mode will be ignored.<br>
> + * This is an implementation of the Strong APA scheduler defined by<br>
> + * Cerqueira et al. in Linux's Processor Affinity API, Refined:<br>
> + * Shifting Real-Time Tasks Towards Higher Schedulability.<br>
repeating text?<br>
<br>
You should add a bit more comment about the high-level design here. Of<br>
course anyone wanting more details can go to the paper.<br></blockquote></div></blockquote><div>Got it. Added a little description in the new patch. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> *<br>
> * @{<br>
> */<br>
><br>
> /**<br>
> - * @brief Scheduler context specialization for Strong APA<br>
> - * schedulers.<br>
> - */<br>
> -typedef struct {<br>
> - Scheduler_SMP_Context Base;<br>
> - Priority_bit_map_Control Bit_map;<br>
> - Chain_Control Ready[ RTEMS_ZERO_LENGTH_ARRAY ];<br>
> -} Scheduler_strong_APA_Context;<br>
> -<br>
> -/**<br>
> - * @brief Scheduler node specialization for Strong APA<br>
> - * schedulers.<br>
> + * @brief Scheduler node specialization for Strong APA schedulers.<br>
> */<br>
> typedef struct {<br>
> /**<br>
> * @brief SMP scheduler node.<br>
> */<br>
> Scheduler_SMP_Node Base;<br>
> +<br>
> + /**<br>
> + * @brief Chain node for Scheduler_strong_APA_Context::All_nodes<br>
> + */<br>
> + Chain_Node Chain;<br>
<br>
Don't call this Chain. We refer to a Chain_Control object as the<br>
chain. Some other ideas:<br>
* APA_Node<br>
* All_nodes_Node<br>
* All_nodes_Link<br>
* Link<br>
* Node (that could be confusing)<br>
* Link_Node<br></blockquote></div></blockquote><div>Renamed to Ready_node. This sounds so better. Thanks </div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I always felt like Joel missed an opportunity to call them Chain Links<br>
instead of Chain Nodes. Then compiler memory fences for synchronizing<br>
them could be Chain Link Fences? (A bit of dad humor.)<br>
<br></blockquote></div></blockquote><div>:p </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + /**<br>
> + * @brief CPU that this node would preempt in the backtracking part of<br>
> + * _Scheduler_strong_APA_Get_highest_ready and<br>
> + * _Scheduler_strong_APA_Do_Enqueue.<br>
> + */<br>
> + Per_CPU_Control *invoker;<br>
<br>
I don't like this name either. What is the invoker invoking? Since it<br>
is used to go backwards, maybe think of a word that conveys that use.<br>
'lowest_scheduled'? 'next_to_preempt'?<br>
<br></blockquote></div></blockquote><div>Changed to cpu_to_preempt, since a node has to preempt the CPU that it is responsible for inserting into the queue while backtracking. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> /**<br>
> - * @brief The associated ready queue of this node.<br>
> + * @brief The associated affinity set of this node.<br>
> */<br>
> - Scheduler_priority_Ready_queue Ready_queue;<br>
> + Processor_mask Affinity;<br>
> } Scheduler_strong_APA_Node;<br>
><br>
> +<br>
> +/**<br>
> + * @brief Struct for each index of the variable size arrays<br>
clarify this comment, no one knows what this means.<br>
Add a period (full stop) after brief sentence.<br>
<br></blockquote></div></blockquote><div>Period added for all subsequent brief doxygen comment. Thanks for pointing it out. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + * its affinity set.<br>
> + */<br>
> + Scheduler_Node *caller;<br>
Not sure what it called. Think about what this name means a bit more.<br></blockquote></div></blockquote><div><br></div><div>Changed to: </div> /**<br> * @brief The node that would preempt this CPU.<br> */ <br><div> Scheduler_Node *preempting_node;</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + /**<br>
> + * @brief Cpu at the index of Scheduler_strong_APA_Context::Struct<br>
Isn't this self-referencing? Just say "CPU in a queue"?<br>
<br></blockquote></div></blockquote><div>Got it,</div><div>Changed to :</div> /**<br> * @brief CPU in a queue.<br> */ <br><div> Per_CPU_Control *cpu; </div><div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + * in Queue implementation.<br>
> + */<br>
> + Per_CPU_Control *cpu;<br>
> +<br>
> + /**<br>
> + * @brief Indicates if the CPU at the index of<br>
> + * Scheduler_strong_APA_Context::Struct is already<br>
> + * added to the Queue or not.<br>
Whether or not this cpu has been added to the queue (visited in the BFS)<br>
<br></blockquote></div></blockquote><div>Done. Thanks. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + */<br>
> + bool visited;<br>
> +} Scheduler_strong_APA_Struct;<br>
<br>
Maybe, Scheduler_strong_APA_BFS_node;<br>
<br></blockquote></div></blockquote><div>I changed it to Scheduler_strong_APA_CPU since for the reasons I mentioned above. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> - Scheduler_strong_APA_Context *self =<br>
> - _Scheduler_strong_APA_Get_self( context );<br>
> - Scheduler_strong_APA_Node *node =<br>
> - _Scheduler_strong_APA_Node_downcast( scheduled_to_ready );<br>
> -<br>
> - _Chain_Extract_unprotected( &node->Base.Base.Node.Chain );<br>
> - _Scheduler_priority_Ready_queue_enqueue_first(<br>
> - &node->Base.Base.Node.Chain,<br>
> - &node->Ready_queue,<br>
> - &self->Bit_map<br>
> + Scheduler_SMP_Node *smp_node;<br>
> + (void) context;<br>
> +<br>
> + smp_node = _Scheduler_SMP_Node_downcast( node );<br>
> + _Scheduler_SMP_Node_update_priority( smp_node, new_priority );<br>
> +}<br>
> +<br>
<br>
Although we don't need doxygen for static (private) methods, a little<br>
bit of comment can be helpful to understand what the helper function<br>
does.<br></blockquote></div></blockquote><div>Added comment.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +static inline bool _Scheduler_strong_APA_Has_ready( Scheduler_Context *context )<br>
> +{<br>
> + Scheduler_strong_APA_Context *self = _Scheduler_strong_APA_Get_self( context );<br>
> +<br>
> + bool ret;<br>
> + const Chain_Node *tail;<br>
> + Chain_Node *next;<br>
> + Scheduler_strong_APA_Node *node;<br>
> +<br>
> + tail = _Chain_Immutable_tail( &self->All_nodes );<br>
> + next = _Chain_First( &self->All_nodes );<br>
> +<br>
> + ret = false;<br>
> +<br>
> + while ( next != tail ) {<br>
> + node = (Scheduler_strong_APA_Node *) STRONG_SCHEDULER_NODE_OF_CHAIN( next );<br>
I see, this only works by chance. fix your macro so it works on purpose. <br></blockquote></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + if (<br>
> + _Scheduler_SMP_Node_state( &node->Base.Base ) == SCHEDULER_SMP_NODE_READY<br>
not enough indent levels<br>
To break this you should use<br>
if (<br>
_Scheduler_SMP_Node_state( &node->Base.Base ) ==<br>
SCHEDULER_SMP_NODE_READY<br>
) {<br>
I know it is kind of ugly.<br>
<br>
> + ) {<br>
> + ret = true;<br>
> + break;<br>
this is fine, but it could also be 'return true;' and then ...<br>
> + }<br>
> +<br>
> + next = _Chain_Next( next );<br>
> + }<br>
> +<br>
> + return ret;<br>
return false;<br>
<br>
Minor nit, but it does simplify your code<br>
<br></blockquote></div></blockquote><div>Yes, indeed it does. Changed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + scheduled = _Scheduler_strong_APA_Node_downcast( scheduled_base );<br>
> +<br>
> + _Scheduler_SMP_Allocate_processor_exact(<br>
> + context,<br>
> + &(scheduled->Base.Base),<br>
> + NULL,<br>
> + victim_cpu<br>
> );<br>
> }<br>
><br>
> -static void _Scheduler_strong_APA_Move_from_ready_to_scheduled(<br>
> +static inline Scheduler_Node * _Scheduler_strong_APA_Find_highest_ready(<br>
> + Scheduler_strong_APA_Context *self,<br>
> + uint32_t front,<br>
> + uint32_t rear<br>
> +)<br>
> +{<br>
> + Scheduler_Node *highest_ready;<br>
> + Scheduler_strong_APA_Struct *Struct;<br>
> + const Chain_Node *tail;<br>
> + Chain_Node *next;<br>
> + uint32_t index_assigned_cpu;<br>
> + Scheduler_strong_APA_Node *node;<br>
> + Priority_Control min_priority_num;<br>
> + Priority_Control curr_priority;<br>
> + Per_CPU_Control *assigned_cpu;<br>
> + Scheduler_SMP_Node_state curr_state;<br>
> + Per_CPU_Control *curr_CPU;<br>
> + bool first_task;<br>
> +<br>
> + Struct = self->Struct;<br>
> + //When the first task accessed has nothing to compare its priority against<br>
> + // So, it is the task with the highest priority witnessed so far!<br>
Use /* */<br>
<br>
> + first_task = true;<br>
> +<br>
> + while( front <= rear ) {<br>
> + curr_CPU = Struct[ front ].cpu;<br>
<br>
Who ensures that rear < sizeof(Struct)?<br>
<br></blockquote></div></blockquote><div>I added an assert. But it should always be the case since we can only insert an element (CPU) if it is not visited before, and there can't be more than CONFIGURE_MAXIMUM_PROCESSOR unvisited CPUs.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + front = front + 1;<br>
> +<br>
> + tail = _Chain_Immutable_tail( &self->All_nodes );<br>
> + next = _Chain_First( &self->All_nodes );<br>
> +<br>
> + while ( next != tail ) {<br>
> + node = (Scheduler_strong_APA_Node*) STRONG_SCHEDULER_NODE_OF_CHAIN( next );<br>
> + //Check if the curr_CPU is in the affinity set of the node<br>
> + if (<br>
> + _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>
> + ) {<br>
extra ws at end of line<br>
you can search a regex like "\s\s*$" for that kind of problem<br>
<br></blockquote></div></blockquote><div>Thanks for telling me this. It is very handy.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + curr_state = _Scheduler_SMP_Node_state( &node->Base.Base );<br>
> +<br>
> + if ( curr_state == SCHEDULER_SMP_NODE_SCHEDULED ) {<br>
> + assigned_cpu = _Thread_Get_CPU( node->Base.Base.user );<br>
> + index_assigned_cpu = _Per_CPU_Get_index( assigned_cpu );<br>
> +<br>
> + if ( Struct[ index_assigned_cpu ].visited == false ) {<br>
> + rear = rear + 1;<br>
> + Struct[ rear ].cpu = assigned_cpu;<br>
> + Struct[ index_assigned_cpu ].visited = true;<br>
OK this is a little bit confusing. You are using the same 'Struct[]'<br>
entry to store metadata for two different CPUs. Somehow you need to<br>
either clarify this, or make it consistent.<br></blockquote></div></blockquote><div>Yes, I noticed it now :p. </div><div><br></div><div>So, The Struct (_Scheduler_Strong_APA_CPU now) stores two things, first is the important variables related to the CPU (at the index) (like visited, preempting_node) but currently it also stores a Per_CPU_Control cpu to act as a queue. Both the parts are independent of each other because they use their indices in the way they want to. This means that front index and rear indexes are used for accessing queue elements, </div><div><br></div><div>So if front = 2, </div><div>CPU[2]. cpu = cpu at front of queue. This could be a cpu with index 5 (say in a 32 processor system) </div><div><br></div><div>but CPU[2]. visited and cpu[2].preempting_node would indicate whether cpu with index 2 is visited or not and point to its preempting node respectively. </div><div><br></div><div>Is it fine if we keep it like this? <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + // The curr CPU of the queue invoked this node to add its CPU<br>
> + // that it is executing on to the queue. So this node might get<br>
> + // preempted because of the invoker curr_CPU and this curr_CPU<br>
> + // is the CPU that node should preempt in case this node<br>
> + // gets preempted.<br>
> + node->invoker = curr_CPU;<br>
> + }<br>
> + }<br>
> + else if ( curr_state == SCHEDULER_SMP_NODE_READY ) {<br>
> + curr_priority = _Scheduler_Node_get_priority( &node->Base.Base );<br>
> + curr_priority = SCHEDULER_PRIORITY_PURIFY( curr_priority );<br>
> +<br>
> + if ( first_task == true || curr_priority < min_priority_num ) {<br>
<br>
you can also initialize min_priority_num to the scheduler's maximum<br>
priority value? Then get rid of this first_task var?<br>
<br></blockquote></div></blockquote><div><br></div><div>While debugging with gdb, I noticed multiple times that the lowest priority ready node (default idle nodes populated at start) have priority of 512 (proof attached as gdb trace at end). I do not know why this is happening. Since I used the STRONG_APA_MAXIMUM_PRIORITY in scheduler.h while defining the scheduler table, shouldn't this be taken care of? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + min_priority_num = curr_priority;<br>
> + highest_ready = &node->Base.Base;<br>
> + first_task = false;<br>
> + //In case this task is directly reachable from thread_CPU<br>
I don't know what this comment means.<br></blockquote></div></blockquote><div>Changed. It is hard to explain this, as I came across the need to do this ( node->invoker = curr_CPU;) myself while debugging. </div><div><br></div><div>I've added the new comment as:</div><div>----------------------------------------------------------------------------------------------------</div><div> /*<br> * In case curr_CPU is filter_CPU, we need to store the<br> * cpu_to_preempt value so that we go back to SMP_*<br> * function, rather than preempting the node ourselves.<br> */<br> node->cpu_to_preempt = curr_CPU;<br></div><div>----------------------------------------------------------------------------------------------------<br></div><div><br></div><div>this is referring to the following line in calling function _Get_highest_ready:</div><div>----------------------------------------------------------------------------------------------------<br></div><div> /*<br> * Highest ready is not just directly reachable from the victim cpu<br> * So there is need of task shifting .<br> */<br> while( node->cpu_to_preempt != filter_cpu ){<br></div><div>----------------------------------------------------------------------------------------------------</div><div><br></div><div>where we check if we need to backtrack or not. </div><div>My comment was trying to explain that for the case the highest ready task is directly accessible from filter_cpu, i.e. the highest ready task has filter_cpu in its affinity set, we need to mark its cpu_to_preempt right then and there so that we don't get a data_exception_error. </div><div><br></div><div>So, there is two cases of preemption that can happen:</div><div><br></div><div>1) a scheduled node preempts another cpu to make space for another node (in the backtracking process): in which case, the same line ( node->cpu_to_preempt = curr_CPU;) is written in the state==...SCHEDULED case above,</div><div><br></div><div>2) the ready node is directly accessible, so the cpu_to_preempt has to be marked right there.</div><div><br></div><div>Is it clear now? Please let me know.</div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + cpu_max = _SMP_Get_processor_maximum();<br>
<br>
I think there is a valid scheduler at cpu_max?<br>
<br></blockquote></div></blockquote><div>When I was debugging, cpu_max was equal to 3 (corresponding to the test case I submitted as patch). So this has to be correct. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + for ( cpu_index = 0 ; cpu_index < cpu_max ; ++cpu_index ) {<br>
So this should be <= not <<br>
Maybe?<br></blockquote></div></blockquote><div><br></div><div>While checking with scope, I found this:</div><div>------------------------------------------------------------<br></div><div> /*<br> * Discover and initialize the secondary cores in an SMP system.<br> */<br><br> cpu_max = _CPU_SMP_Initialize();<br> cpu_max = cpu_max < cpu_config_max ? cpu_max : cpu_config_max;<br> _SMP_Processor_maximum = cpu_max;<br></div><div>------------------------------------------------------------<br></div><div><br></div><div>So, the cpu_max stores the actually number of cpus, and since we use 0 index for per_cpus we use < and not <=. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + Struct[ cpu_index ].visited = false;<br>
> + }<br>
> +<br>
> + rear = rear + 1;<br>
why not just init rear to 0?<br></blockquote></div></blockquote><div>Okay. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + Struct[ rear ].cpu = filter_cpu;<br>
> + Struct[ _Per_CPU_Get_index( filter_cpu ) ].visited = true;<br>
> +<br>
> + highest_ready = _Scheduler_strong_APA_Find_highest_ready(<br>
> + self,<br>
> + front,<br>
> + rear<br>
> + );<br>
wrong indents<br></blockquote></div></blockquote><div><br></div><div>It looks fine in my code editor (on <a href="https://github.com/richidubey/rtems/blob/4bbf5d9abe1e16bd942b9806e0be7efcd0cbfe7f/cpukit/score/src/schedulerstrongapa.c#L279">github </a>too), but it looks like this on patch and when I try pasting it here too. Why does this happen? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + _Scheduler_SMP_Preempt(<br>
> + context,<br>
> + curr_node,<br>
> + _Thread_Scheduler_get_home_node( node->invoker->heir ),<br>
> + _Scheduler_strong_APA_Allocate_processor<br>
> + );<br>
> +<br>
> + node = _Scheduler_strong_APA_Node_downcast( next_node );<br>
> + }<br>
This is repetitive code, can you merge the first part of the 'if'<br>
block into the while loop?<br></blockquote></div></blockquote><div>Changed. Thanks </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + if ( curr_priority > max_priority_num ) {<br>
> + lowest_scheduled = curr_node;<br>
> + max_priority_num = curr_priority;<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + return lowest_scheduled;<br>
<br>
Is it possible this is NULL? what happens if it is?<br>
<br></blockquote></div></blockquote><div>I've now added an assert to check. But the earlier <a href="https://github.com/richidubey/rtems/blob/4bbf5d9abe1e16bd942b9806e0be7efcd0cbfe7f/cpukit/score/src/schedulerstrongapa.c#L347">assert</a> should make sure it should not be NULL since if the affinity is not 0, the lowest scheduled would be the node with the minimum priority among all the processors in its affinity. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> + _Assert( !_Chain_Is_empty(self->All_nodes) );<br>
> + _Assert( !_Chain_Is_node_off_chain( &node->Chain ) );<br>
> +<br>
> + _Chain_Extract_unprotected( &node->Chain ); //Removed from All_nodes<br>
<br>
All_nodes name is now confusing me. I thought maybe it meant blocked<br>
nodes too, but I understand now.<br>
<br>
You can call the All_nodes chain 'Ready'. It should be OK to leave<br>
Executing tasks on a Ready structure. That is how the uniprocessor<br>
schedulers work as I recall.<br>
<br></blockquote></div></blockquote><div>Ready sounds great. Changed. Thanks</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + if ( !curr_thread->is_idle ) {<br>
> + for ( cpu_index = 0 ; cpu_index < cpu_max ; ++cpu_index ) {<br>
<= ?<br>
<br></blockquote></div></blockquote><div>I believe it should be <. Cause the test passes and I found no problems while debugging as well.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + if ( _Processor_mask_Is_set( &curr_strong_node->Affinity, cpu_index ) ) {<br>
> + //Checks if the thread_CPU is in the affinity set of the node<br>
> + Per_CPU_Control *cpu = _Per_CPU_Get_by_index( cpu_index );<br>
> + if ( _Per_CPU_Is_processor_online( cpu ) && Struct[ cpu_index ].visited == false ) {<br>
> + rear = rear + 1;<br>
> + Struct[ rear ].cpu = cpu;<br>
> + Struct[ cpu_index ].visited = true;<br>
> + Struct[ cpu_index ].caller = curr_node;<br>
<br>
maybe instead of caller it should be 'ancestor' or something similar<br>
to denote the ordering.<br>
<br></blockquote></div></blockquote><div>Yes, now it is preempting_node. Thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + node = _Scheduler_strong_APA_Node_downcast( node_base );<br>
> + affinity = arg;<br>
> + node->Affinity = *affinity;<br>
can simplify to:<br>
node->Affinity = (const Processor_mask *)arg;<br>
<br></blockquote></div></blockquote><div>Changed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> _Scheduler_strong_APA_Enqueue_scheduled,<br>
> - _Scheduler_SMP_Do_nothing_register_idle<br>
> + _Scheduler_strong_APA_Register_idle<br>
why not use the Do_nothing?<br>
<br></blockquote></div></blockquote><div>Changed. <br></div><div><br></div><div>Once again, Thanks a lot for your reviews. A lot of code got improved which wouldn't have been possible without your review. </div><div><br></div><div>-----------------------------------------End of my comments--------------------------------------------------</div><div><br></div><div>gdb trace:</div><div><br></div><div>--------------------------------------------------------------</div><div>richi@YouAreAmazing:~/quick-start/rtems/5/bin$ ./arm-rtems5-gdb --command=arm.gdb ~/quick-start/build/b3-realview/arm-rtems5/c/realview_pbx_a9_qemu/testsuites/smptests/smpstrongapa01.exe<br>...</div><div>Loading section .rtemsroset, size 0x74 lma 0x121384<br>Loading section .data, size 0x530 lma 0x200000<br>Start address 0x00100040, load size 137476<br>Transfer rate: 12204 KB/sec, 1808 bytes/write.<br>(gdb) continue<br>Continuing.<br><br>Thread 1 hit Breakpoint 6, Init (arg=2113828) at /home/richi/quick-start/src/rtems/c/src/../../testsuites/smptests/smpstrongapa01/init.c:328<br>328 TEST_BEGIN();<br>(gdb) <br>Continuing.<br><br>Thread 1 hit Breakpoint 5, _Terminate (the_source=RTEMS_FATAL_SOURCE_EXIT, the_error=0) at /home/richi/quick-start/src/rtems/c/src/../../cpukit/score/src/interr.c:36<br>36 _User_extensions_Fatal( the_source, the_error );<br>(gdb) <br>Continuing.<br><br>Thread 1 hit Breakpoint 4, bsp_reset () at /home/richi/quick-start/src/rtems/c/src/lib/libbsp/arm/realview-pbx-a9/../../../../../../bsps/arm/realview-pbx-a9/start/bspreset.c:19<br>19 volatile uint32_t *sys_lock = (volatile uint32_t *) 0x10000020;<br>(gdb) <br>Continuing.<br><br>Thread 1 hit Breakpoint 6, Init (arg=2113828) at /home/richi/quick-start/src/rtems/c/src/../../testsuites/smptests/smpstrongapa01/init.c:328<br>328 TEST_BEGIN();<br>(gdb) b test<br>Breakpoint 7 at 0x10109c: file /home/richi/quick-start/src/rtems/c/src/../../testsuites/smptests/smpstrongapa01/init.c, line 286.<br>(gdb) continue<br>Continuing.<br><br>Thread 1 hit Breakpoint 7, test () at /home/richi/quick-start/src/rtems/c/src/../../testsuites/smptests/smpstrongapa01/init.c:286<br>286 ctx = &test_instance;<br>(gdb) b _Scheduler_strong_APA_Find_highest_ready<br>Breakpoint 8 at 0x118922: file /home/richi/quick-start/src/rtems/c/src/../../cpukit/score/src/schedulerstrongapa.c, line 153.<br>(gdb) continue<br>Continuing.<br><br>Thread 1 hit Breakpoint 8, _Scheduler_strong_APA_Find_highest_ready (self=0x200620 <_Configuration_Scheduler_strong_APA_dflt>, front=0, rear=0) at /home/richi/quick-start/src/rtems/c/src/../../cpukit/score/src/schedulerstrongapa.c:153<br>153 CPU = self->CPU;<br>(gdb) ni<br>0x00118924 153 CPU = self->CPU;<br>(gdb) <br>0x00118926 153 CPU = self->CPU;<br>(gdb) <br>158 first_task = true;<br>(gdb) <br>0x0011892a 158 first_task = true;<br>(gdb) <br>163 while( front <= rear ) {<br>(gdb) <br>163 while( front <= rear ) {<br>(gdb) <br>0x00118a6e 163 while( front <= rear ) {<br>(gdb) <br>0x00118a70 163 while( front <= rear ) {<br>(gdb) <br>0x00118a72 163 while( front <= rear ) {<br>(gdb) <br>164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x00118932 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x00118934 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x00118936 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x00118938 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x0011893a 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x0011893c 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x0011893e 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x00118940 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>0x00118942 164 curr_CPU = CPU[ front ].cpu;<br>(gdb) <br>165 front = front + 1;<br>(gdb) <br>0x00118946 165 front = front + 1;<br>(gdb) <br>0x00118948 165 front = front + 1;<br>(gdb) <br>167 tail = _Chain_Immutable_tail( &self->Ready );<br>(gdb) <br>0x0011894c 167 tail = _Chain_Immutable_tail( &self->Ready );<br>(gdb) <br>0x0011894e 167 tail = _Chain_Immutable_tail( &self->Ready );<br>(gdb) <br>0x00118950 167 tail = _Chain_Immutable_tail( &self->Ready );<br>(gdb) <br>0x00118954 167 tail = _Chain_Immutable_tail( &self->Ready );<br>(gdb) <br>168 next = _Chain_First( &self->Ready );<br>(gdb) <br>0x00118958 168 next = _Chain_First( &self->Ready );<br>(gdb) <br>0x0011895a 168 next = _Chain_First( &self->Ready );<br>(gdb) <br>0x0011895c 168 next = _Chain_First( &self->Ready );<br>(gdb) <br>0x00118960 168 next = _Chain_First( &self->Ready );<br>(gdb) <br>170 while ( next != tail ) {<br>(gdb) <br>170 while ( next != tail ) {<br>(gdb) <br>0x00118a64 170 while ( next != tail ) {<br>(gdb) <br>0x00118a66 170 while ( next != tail ) {<br>(gdb) <br>0x00118a68 170 while ( next != tail ) {<br>(gdb) <br>171 node = (Scheduler_strong_APA_Node*) STRONG_SCHEDULER_NODE_OF_CHAIN( next );<br>(gdb) <br>0x00118966 171 node = (Scheduler_strong_APA_Node*) STRONG_SCHEDULER_NODE_OF_CHAIN( next );<br>(gdb) <br>0x00118968 171 node = (Scheduler_strong_APA_Node*) STRONG_SCHEDULER_NODE_OF_CHAIN( next );<br>(gdb) <br>174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x0011896c 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x00118970 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x00118972 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x00118976 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x00118978 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x0011897a 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x0011897c 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>0x00118980 174 _Processor_mask_Is_set(&node->Affinity, _Per_CPU_Get_index(curr_CPU))<br>(gdb) <br>173 if (<br>(gdb) <br>0x00118984 173 if (<br>(gdb) <br>176 curr_state = _Scheduler_SMP_Node_state( &node->Base.Base );<br>(gdb) <br>0x00118988 176 curr_state = _Scheduler_SMP_Node_state( &node->Base.Base );<br>(gdb) <br>0x0011898a 176 curr_state = _Scheduler_SMP_Node_state( &node->Base.Base );<br>(gdb) <br>0x0011898e 176 curr_state = _Scheduler_SMP_Node_state( &node->Base.Base );<br>(gdb) <br>178 if ( curr_state == SCHEDULER_SMP_NODE_SCHEDULED ) {<br>(gdb) <br>0x00118992 178 if ( curr_state == SCHEDULER_SMP_NODE_SCHEDULED ) {<br>(gdb) <br>0x00118994 178 if ( curr_state == SCHEDULER_SMP_NODE_SCHEDULED ) {<br>(gdb) <br>196 else if ( curr_state == SCHEDULER_SMP_NODE_READY ) {<br>(gdb) <br>0x00118a00 196 else if ( curr_state == SCHEDULER_SMP_NODE_READY ) {<br>(gdb) <br>0x00118a02 196 else if ( curr_state == SCHEDULER_SMP_NODE_READY ) {<br>(gdb) <br>197 curr_priority = _Scheduler_Node_get_priority( &node->Base.Base );<br>(gdb) <br>0x00118a06 197 curr_priority = _Scheduler_Node_get_priority( &node->Base.Base );<br>(gdb) <br>0x00118a08 197 curr_priority = _Scheduler_Node_get_priority( &node->Base.Base );<br>(gdb) <br>0x00118a0c 197 curr_priority = _Scheduler_Node_get_priority( &node->Base.Base );<br>(gdb) <br>198 curr_priority = SCHEDULER_PRIORITY_PURIFY( curr_priority );<br>(gdb) <br>0x00118a14 198 curr_priority = SCHEDULER_PRIORITY_PURIFY( curr_priority );<br>(gdb) <br>0x00118a18 198 curr_priority = SCHEDULER_PRIORITY_PURIFY( curr_priority );<br>(gdb) <br>0x00118a1c 198 curr_priority = SCHEDULER_PRIORITY_PURIFY( curr_priority );<br>(gdb) <br>0x00118a20 198 curr_priority = SCHEDULER_PRIORITY_PURIFY( curr_priority );<br>(gdb) <br>0x00118a24 198 curr_priority = SCHEDULER_PRIORITY_PURIFY( curr_priority );<br>(gdb) <br>200 if ( first_task == true || curr_priority < min_priority_num ) {<br>(gdb) p curr_priority<br>$1 = 510<br>(gdb) p first_task<br>$2 = true<br>(gdb) <br></div><div>----------------------------------------------------------------------------------------------------------------------------<br></div><div><br></div></div></div>