[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1585: sched/task: do not migrate the task state to INVALID

[GitBox <gi...@apache.org>]

xiaoxiang781216 commented on pull request #1585:
URL: https://github.com/apache/incubator-nuttx/pull/1585#issuecomment-684157177


   > > @patacongo do you want:
   > > 1.Move line 158-161 after line 188
   > > 2.Or move line 177-188 before line 158
   > > But both modification will introduce the huge change of termination sequence. Since tcb is freed immediately at line 199, the change made by @anchao is reasonable.
   > 
   > I actually do not have any strong opinion. I do not fully understand either the problem or the solution. My only point is that disassociating task_state from the list that the TCB resides in is very dangerous. At least this danger should be documenting fully and clearly in comments so that no one seriously breaks things in the future.
   
   The problem is that if a thread is waiting for a message queue and other thread call pthread_cancel on it:
   1.nxtask_terminate change the state to TSTATE_TASK_INVALID
   2.and then call nxmq_recover to cleanup the message queue state
   3.nxmq_recover skip all actions because the thread's state isn't match:
   ```
     /* Was the task waiting for a message queue to become non-empty? */
   
     if (tcb->task_state == TSTATE_WAIT_MQNOTEMPTY)
       {
         /* Decrement the count of waiters */
   
         DEBUGASSERT(tcb->msgwaitq && tcb->msgwaitq->nwaitnotempty > 0);
         tcb->msgwaitq->nwaitnotempty--;
       }
   ```  
   then, the message queue enter an inconsistent state and will panic with any mq_xxx API.
   Actually, this patch is regressioned by your commit:
   ```
   commit e24f2814015c6dcd9fc67edcd399ccbaf41c3669
   Author: Gregory Nutt <gn...@nuttx.org>
   Date:   Sun Nov 20 07:57:18 2016 -0600
   
       This commit adds a new internal interfaces and fixes a problem with three APIs in the SMP configuration.  The new internal interface is sched_cpu_p
   ause(tcb).  This function will pause a CPU if the task associated with 'tcb' is running on that CPU.  This allows a different CPU to modify that OS dat
   a stuctures associated with the CPU.  When the other CPU is resumed, those modifications can safely take place.
       
       The three fixes are to handle cases in the SMP configuration where one CPU does need to make modifications to TCB and data structures on a task tha
   t could be running running on another CPU.  Those three cases are task_delete(), task_restart(), and execution of signal handles.  In all three cases t
   he solutions is basically the same:  (1) Call sched_cpu_pause(tcb) to pause the CPU on which the task is running, (2) perform the necessary operations,
    then (3) call up_cpu_resume() to restart the paused CPU.
   ```
   
   > 
   > The meaning of task_state is the list that the TCB resides in. The PR breaks that meaning.
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org