xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074663527
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -59,17 +59,37 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
bool sync,
*/
flags = spin_lock_irqsave(&wqueue->lock);
- if (work->worker != NULL)
+
+ /* Check whether we own the work structure. */
+
+ if (!work_available(work))
{
- /* Remove the entry from the work queue and make sure that it is
- * marked as available (i.e., the worker field is nullified).
- */
+ bool is_head = list_is_head(&wqueue->pending, &work->node);
+
+ /* Seize the ownership from the work thread. */
work->worker = NULL;
- wd_cancel(&work->u.timer);
- list_delete(&work->u.s.node);
- ret = OK;
+ list_delete(&work->node);
+
+ /* If the head of the pending queue has changed, we should reset
+ * the wqueue timer.
+ */
+
+ if (is_head)
+ {
+ if (!list_is_empty(&wqueue->expired))
Review Comment:
no change
##########
sched/wqueue/kwork_thread.c:
##########
@@ -148,33 +193,54 @@ static int work_thread(int argc, FAR char *argv[])
kworker = (FAR struct kworker_s *)
((uintptr_t)strtoul(argv[2], NULL, 16));
- flags = spin_lock_irqsave(&wqueue->lock);
- sched_lock();
-
- /* Loop forever */
+ /* Loop until wqueue->exit != 0.
+ * Since the only way to set wqueue->exit is to call work_queue_free(),
+ * there is no need for entering the critical section.
+ */
while (!wqueue->exit)
{
- /* And check each entry in the work queue. Since we have disabled
+ /* And check first entry in the work queue. Since we have disabled
* interrupts we know: (1) we will not be suspended unless we do
* so ourselves, and (2) there will be no changes to the work queue
*/
- /* Remove the ready-to-execute work from the list */
+ flags = spin_lock_irqsave(&wqueue->lock);
+ sched_lock();
+
+ /* If the wqueue timer is expired and non-active, it indicates that
+ * there might be expired work in the pending queue.
+ */
- while (!list_is_empty(&wqueue->q))
+ if (!WDOG_ISACTIVE(&wqueue->timer))
{
- work = list_first_entry(&wqueue->q, struct work_s, u.s.node);
+ unsigned int wake_count = work_dispatch(wqueue);
- list_delete(&work->u.s.node);
+ spin_unlock_irqrestore(&wqueue->lock, flags);
+ sched_unlock();
+
+ /* Note that the thread execution this function is also
+ * a worker thread, which has already been woken up by the timer.
+ * So only `count - 1` semaphore will be posted.
+ */
- if (work->worker == NULL)
+ while (wake_count-- > 1)
{
- continue;
+ nxsem_post(&wqueue->sem);
Review Comment:
but it isn't truth, since sched_lock is holding during the whole life time
of work_dispatch
##########
sched/wqueue/wqueue.h:
##########
@@ -159,6 +148,65 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+/****************************************************************************
+ * Name: work_insert_pending
+ *
+ * Description:
+ * Internal public function to insert the work to 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 work is inserted at the head of the pending queue.
+ *
+ ****************************************************************************/
+
+static inline_function
+bool work_insert_pending(FAR struct kwork_wqueue_s *wqueue,
+ FAR struct work_s *work)
+{
+ struct work_s *curr;
+
+ DEBUGASSERT(wqueue != NULL && work != NULL);
+
+ /* Insert the work into the wait queue sorted by the expired time. */
+
+ list_for_every_entry(&wqueue->pending, curr, struct work_s, node)
+ {
+ if (!clock_compare(curr->qtime, work->qtime))
+ {
+ break;
+ }
+ }
+
+ /* After the insertion, we do not violate the invariant that
+ * the wait queue is sorted by the expired time. Because
+ * curr->qtime > work_qtime.
+ * In the case of the wqueue is empty, we insert
+ * the work at the head of the wait queue.
+ */
+
+ list_add_before(&curr->node, &work->node);
+
+ return &curr->node == &wqueue->pending;
Review Comment:
but you always touch work at line 192
--
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.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
