This is an automated email from the ASF dual-hosted git repository.

archer pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git

commit f442a41102deaabbd06d78ea4c9736f33635678f
Author: ouyangxiangzhen <ouyangxiangz...@xiaomi.com>
AuthorDate: Thu May 8 11:17:46 2025 +0800

    sched/wqueue: Fix work_cancel_sync.
    
    This commit fixed work_cancel_sync at a very rare boundary case. When a 
worker thread re-enqueues the work data structure during the execution of work, 
the user thread cannot directly dequeue the work in work_cancel_sync. Instead, 
it should wait until all workers' references to the work data structure have 
been eliminated after dequeuing.
    
    Signed-off-by: ouyangxiangzhen <ouyangxiangz...@xiaomi.com>
---
 sched/wqueue/kwork_cancel.c | 53 +++++++++++++++---------------------
 sched/wqueue/wqueue.h       | 66 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 31 deletions(-)

diff --git a/sched/wqueue/kwork_cancel.c b/sched/wqueue/kwork_cancel.c
index 369730d6a0..4a2019ce53 100644
--- a/sched/wqueue/kwork_cancel.c
+++ b/sched/wqueue/kwork_cancel.c
@@ -46,7 +46,7 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue, 
bool sync,
                         FAR struct work_s *work)
 {
   irqstate_t flags;
-  int        ret = OK;
+  FAR sem_t *sync_wait = NULL;
 
   if (wqueue == NULL || work == NULL)
     {
@@ -60,56 +60,49 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue, 
bool sync,
 
   flags = spin_lock_irqsave(&wqueue->lock);
 
-  /* Check whether we own the work structure. */
-
   if (!work_available(work))
     {
-      bool is_head = list_is_head(&wqueue->pending, &work->node);
-
-      /* Seize the ownership from the work thread. */
-
-      work->worker = NULL;
-
-      list_delete(&work->node);
-
       /* If the head of the pending queue has changed, we should reset
        * the wqueue timer.
        */
 
-      if (is_head)
+      if (work_remove(wqueue, work))
         {
-          if (!list_is_empty(&wqueue->pending))
-            {
-              work = list_first_entry(&wqueue->pending, struct work_s, node);
-
-              ret = wd_start_abstick(&wqueue->timer, work->qtime,
-                                     work_timer_expired, (wdparm_t)wqueue);
-            }
-          else
-            {
-              wd_cancel(&wqueue->timer);
-            }
+          work_timer_reset(wqueue);
         }
     }
-  else if (!up_interrupt_context() && !sched_idletask() && sync)
+
+  /* Note that cancel_sync can not be called in the interrupt
+   * context and the idletask context.
+   */
+
+  if (sync)
     {
       int wndx;
+      pid_t pid = nxsched_gettid();
+
+      /* Wait until the worker thread finished the work. */
 
       for (wndx = 0; wndx < wqueue->nthreads; wndx++)
         {
           if (wqueue->worker[wndx].work == work &&
-              wqueue->worker[wndx].pid != nxsched_gettid())
+              wqueue->worker[wndx].pid != pid)
             {
               wqueue->worker[wndx].wait_count++;
-              spin_unlock_irqrestore(&wqueue->lock, flags);
-              nxsem_wait_uninterruptible(&wqueue->worker[wndx].wait);
-              return 1;
+              sync_wait = &wqueue->worker[wndx].wait;
+              break;
             }
         }
     }
 
   spin_unlock_irqrestore(&wqueue->lock, flags);
-  return ret;
+
+  if (sync_wait)
+    {
+      nxsem_wait_uninterruptible(sync_wait);
+    }
+
+  return 0;
 }
 
 /****************************************************************************
@@ -163,8 +156,6 @@ int work_cancel_wq(FAR struct kwork_wqueue_s *wqueue,
  *
  * Returned Value:
  *   Zero means the work was successfully cancelled.
- *   One means the work was not cancelled because it is currently being
- *   processed by work thread, but wait for it to finish.
  *   A negated errno value is returned on any failure:
  *
  *   -ENOENT - There is no such work queued.
diff --git a/sched/wqueue/wqueue.h b/sched/wqueue/wqueue.h
index c6ecb95883..57177ad98d 100644
--- a/sched/wqueue/wqueue.h
+++ b/sched/wqueue/wqueue.h
@@ -203,6 +203,39 @@ bool work_insert_pending(FAR struct kwork_wqueue_s *wqueue,
   return curr == head;
 }
 
+/****************************************************************************
+ * Name: work_remove
+ *
+ * Description:
+ *   Internal public function to remove the work from the workqueue.
+ *   Require wqueue != NULL and work != NULL.
+ *
+ * Input Parameters:
+ *   wqueue - The work queue.
+ *   work   - The work to be inserted.
+ *
+ * Returned Value:
+ *   Return whether the head of the pending queue has changed.
+ *
+ ****************************************************************************/
+
+static inline_function
+bool work_remove(FAR struct kwork_wqueue_s *wqueue,
+                 FAR struct work_s         *work)
+{
+  FAR struct work_s *head;
+
+  head = list_first_entry(&wqueue->pending, struct work_s, node);
+
+  /* Seize the ownership from the work thread. */
+
+  work->worker = NULL;
+
+  list_delete(&work->node);
+
+  return head == work;
+}
+
 /****************************************************************************
  * Name: work_timer_expired
  *
@@ -216,6 +249,39 @@ bool work_insert_pending(FAR struct kwork_wqueue_s *wqueue,
 
 void work_timer_expired(wdparm_t arg);
 
+/****************************************************************************
+ * Name: work_timer_reset
+ *
+ * Description:
+ *   Internal public function to reset the timer of the workqueue.
+ *   Require wqueue != NULL.
+ *
+ * Input Parameters:
+ *   wqueue - The work queue.
+ *
+ * Returned Value:
+ *   None.
+ *
+ ****************************************************************************/
+
+static inline_function
+void work_timer_reset(FAR struct kwork_wqueue_s *wqueue)
+{
+  if (!list_is_empty(&wqueue->pending))
+    {
+      FAR struct work_s *work;
+
+      work = list_first_entry(&wqueue->pending, struct work_s, node);
+
+      wd_start_abstick(&wqueue->timer, work->qtime,
+                       work_timer_expired, (wdparm_t)wqueue);
+    }
+  else
+    {
+      wd_cancel(&wqueue->timer);
+    }
+}
+
 /****************************************************************************
  * Name: work_start_highpri
  *

Reply via email to