CV-Bowen commented on code in PR #21385:
URL: https://github.com/apache/nuttx/pull/21385#discussion_r4450563359
##########
sched/wqueue/kwork_thread.c:
##########
@@ -117,49 +117,144 @@ struct lp_wqueue_s g_lpwork =
* Private Functions
****************************************************************************/
-static inline_function
-void work_dispatch(FAR struct kwork_wqueue_s *wq)
+/****************************************************************************
+ * Name: work_dispatch
+ *
+ * Description:
+ * Move the expired work to the expired queue and run all of it on the
+ * given worker thread.
+ *
+ * Input Parameters:
+ * wqueue - The work queue.
+ * kworker - The worker thread.
+ *
+ * Returned Value:
+ * true if the work queue is being freed and the thread should exit.
+ *
+ ****************************************************************************/
+
+static bool work_dispatch(FAR struct kwork_wqueue_s *wqueue,
+ FAR struct kworker_s *kworker)
{
FAR struct work_s *work;
FAR struct work_s *next;
unsigned int count = 0;
clock_t ticks = clock_systime_ticks();
+ irqstate_t flags;
+ worker_t worker;
+ FAR void *arg;
- /* Wake up the worker thread once there is expired work.
- * If some worker threads are busy, here the callback will
- * wake up another waiting work thread.
- *
- * Becareful of the special case that the pending work
- * has been canceled but the timer is expired.
- * In this case we should not wake up any worker thread.
+ /* 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
*/
- list_for_every_entry_safe(&wq->pending, work, next, struct work_s, node)
+ flags = spin_lock_irqsave_nopreempt(&wqueue->lock);
+
+ if (wqueue->exit)
+ {
+ spin_unlock_irqrestore_nopreempt(&wqueue->lock, flags);
+ return true;
+ }
+
+ /* If the wqueue timer is expired and non-active, it indicates that
+ * there might be expired work in the pending queue.
+ */
+
+ if (!WDOG_ISACTIVE(&wqueue->timer))
{
- /* Check whether the work has expired. */
+ /* Wake up the worker thread once there is expired work.
+ * If some worker threads are busy, here the callback will
+ * wake up another waiting work thread.
+ *
+ * Becareful of the special case that the pending work
+ * has been canceled but the timer is expired.
+ * In this case we should not wake up any worker thread.
+ */
- if (!clock_compare(work->qtime, ticks))
+ list_for_every_entry_safe(&wqueue->pending, work, next,
+ struct work_s, node)
{
- wd_start_abstick(&wq->timer, work->qtime,
- work_timer_expired, (wdparm_t)wq);
- break;
+ /* Check whether the work has expired. */
+
+ if (!clock_compare(work->qtime, ticks))
+ {
+ wd_start_abstick(&wqueue->timer, work->qtime,
+ work_timer_expired, (wdparm_t)wqueue);
+ break;
+ }
+
+ /* Expired work will be moved to tail of the expired queue. */
+
+ list_delete(&work->node);
+ list_add_tail(&wqueue->expired, &work->node);
+
+ /* 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 (count++ > 0)
+ {
+ nxsem_post(&wqueue->sem);
+ }
}
+ }
- /* Expired work will be moved to tail of the expired queue. */
+ while (!list_is_empty(&wqueue->expired))
+ {
+ work = list_first_entry(&wqueue->expired, struct work_s, node);
list_delete(&work->node);
- list_add_tail(&wq->expired, &work->node);
- /* 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.
+ /* Extract the work description from the entry (in case the
+ * work instance will be reused after it has been de-queued).
+ */
+
+ worker = work->worker;
+
+ /* Extract the work argument (before re-enabling interrupts) */
+
+ arg = work->arg;
+
+ /* Return the work structure ownership to the work owner. */
+
+ work->worker = NULL;
+
+ /* Mark the thread busy */
+
+ kworker->work = work;
+
+ spin_unlock_irqrestore_nopreempt(&wqueue->lock, flags);
+
+ /* Do the work. Re-enable interrupts while the work is being
+ * performed... we don't have any idea how long this will take!
*/
- if (count++ > 0)
+ CALL_WORKER(worker, arg);
+ flags = spin_lock_irqsave_nopreempt(&wqueue->lock);
+
+ /* Mark the thread un-busy */
+
+ kworker->work = NULL;
Review Comment:
Fixed: work_dispatch() saves kworker->work on entry and restores it after
each callback. With your sequence (outer queues inner, calls
work_queue_dispatch(), then sleeps) on sim:smp, work_cancel_sync_wq(outer)
returned -2 before and now returns 0 after waiting for the callback. ostest
wqueue still passes.
##########
include/nuttx/wqueue.h:
##########
@@ -356,6 +356,20 @@ FAR struct kwork_wqueue_s *work_queue_create(FAR const
char *name,
FAR void *stack_addr,
int stack_size, int nthreads);
+/****************************************************************************
+ * Name: work_queue_dispatch
+ *
+ * Description:
+ * Execute all pending work items on a given work queue by iterating
+ * through each worker thread and invoking its execution routine.
+ *
+ * Input Parameters:
+ * wqueue - Work queue instance.
+ *
+ ****************************************************************************/
+
+void work_queue_dispatch(FAR struct kwork_wqueue_s *wqueue);
Review Comment:
Fixed: the three functions are declared only for CONFIG_BUILD_FLAT ||
__KERNEL__; they serve kernel drivers only.
##########
include/nuttx/wqueue.h:
##########
@@ -356,6 +356,20 @@ FAR struct kwork_wqueue_s *work_queue_create(FAR const
char *name,
FAR void *stack_addr,
int stack_size, int nthreads);
+/****************************************************************************
+ * Name: work_queue_dispatch
+ *
+ * Description:
+ * Execute all pending work items on a given work queue by iterating
Review Comment:
Fixed: the comment at both places now says it runs the expired work on the
calling thread, only when the caller is one of the queue's workers, and does
nothing otherwise.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
