casaroli commented on code in PR #21385:
URL: https://github.com/apache/nuttx/pull/21385#discussion_r4448849828
##########
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:
When a callback calls `work_queue_dispatch()`, the nested `work_dispatch()`
sets `kworker->work` to NULL here, but the outer callback still runs.
`work_cancel_sync()` on the outer work then finds no worker and returns -ENOENT
at once (tested on qemu-armv8a:nsh: outer queues inner, calls
`work_queue_dispatch()`, then sleeps; `work_cancel_sync_wq(outer)` returns -2
while outer runs).
Fix: in `work_dispatch()` save `kworker->work` on entry and restore that
value here instead of NULL.
##########
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:
These functions are declared for user space too (this header serves
libs/libc/wqueue in protected and kernel builds), but only sched/wqueue
implements them, so a user program there does not link.
Fix: implement them in libs/libc/wqueue/work_usrthread.c, or declare them
only for `CONFIG_BUILD_FLAT || __KERNEL__`.
##########
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:
`work_queue_dispatch()` runs only the expired work, only on the calling
thread, and does nothing when the caller is not a worker of `wqueue`; it does
not iterate through each worker thread.
Fix: say this here and at kwork_thread.c:421.
--
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]
