xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050582467
##########
sched/wqueue/wqueue.h:
##########
@@ -66,14 +66,15 @@ struct kworker_s
struct kwork_wqueue_s
{
- struct list_node q; /* The queue of pending work */
- sem_t sem; /* The counting semaphore of the wqueue */
- sem_t exsem; /* Sync waiting for thread exit */
- spinlock_t lock; /* Spinlock */
- uint8_t nthreads; /* Number of worker threads */
- bool exit; /* A flag to request the thread to exit */
- int16_t wait_count;
- struct kworker_s worker[0]; /* Describes a worker thread */
+ struct list_node wait_q; /* The queue of pending periodical work. */
Review Comment:
let's keep original name(`q`)
##########
sched/wqueue/kwork_thread.c:
##########
@@ -136,10 +182,12 @@ static int work_thread(int argc, FAR char *argv[])
{
FAR struct kwork_wqueue_s *wqueue;
FAR struct kworker_s *kworker;
- FAR struct work_s *work;
- worker_t worker;
- irqstate_t flags;
- FAR void *arg;
+ FAR struct work_s *work;
Review Comment:
remove extra spaces
##########
sched/wqueue/kwork_thread.c:
##########
@@ -110,6 +114,48 @@ struct lp_wqueue_s g_lpwork =
* Private Functions
****************************************************************************/
+/* The work time expired callback will wake up the worker thread. */
+
+static void work_timer_expired(wdparm_t arg)
+{
+ FAR struct kwork_wqueue_s *wq = (FAR struct kwork_wqueue_s *)arg;
+ irqstate_t flags = spin_lock_irqsave(&wq->lock);
+ sched_lock();
+
+ if (wq->wait_count > 0)
Review Comment:
let's create a new patch to change wait_count to atomic and remove the
spinlock around it
##########
sched/wqueue/kwork_thread.c:
##########
@@ -394,6 +493,8 @@ int work_queue_free(FAR struct kwork_wqueue_s *wqueue)
/* Mark the work queue as exiting */
+ wd_cancel(&wqueue->timer);
+
Review Comment:
remove the blank line
##########
sched/wqueue/kwork_thread.c:
##########
@@ -148,30 +196,35 @@ 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();
+ list_initialize(&q);
- /* Loop forever */
+ /* Loop until wqueue->exit != 0.
+ * We do not need to enter the critical-section to access wqueue->exit.
+ * Because the only way to set wqueue->exit is to call work_queue_free().
+ */
while (!wqueue->exit)
{
+ irqstate_t flags = spin_lock_irqsave(&wqueue->lock);
+ sched_lock();
+
/* And check each 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
*/
+ /* Check if there is expired delayed work */
+
+ ticks = clock_systime_ticks();
+ work_expiration(wqueue, &q, ticks);
Review Comment:
work_cancel can't find the work in the local list(`q`)
##########
sched/wqueue/kwork_thread.c:
##########
@@ -148,30 +196,35 @@ 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();
+ list_initialize(&q);
- /* Loop forever */
+ /* Loop until wqueue->exit != 0.
+ * We do not need to enter the critical-section to access wqueue->exit.
+ * Because the only way to set wqueue->exit is to call work_queue_free().
+ */
while (!wqueue->exit)
{
+ irqstate_t flags = spin_lock_irqsave(&wqueue->lock);
+ sched_lock();
Review Comment:
remove, don't need sched_lock
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -59,37 +61,36 @@ 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).
- */
+ /* Seize the ownership from the work thread. */
+ worker = work->worker;
+ arg = work->arg;
+
+ run_myself = sync;
work->worker = NULL;
- wd_cancel(&work->u.timer);
- list_delete(&work->u.s.node);
- ret = OK;
+ list_delete(&work->node);
}
- else if (!up_interrupt_context() && !sched_idletask() && sync)
+
+ spin_unlock_irqrestore(&wqueue->lock, flags);
+
+ if (run_myself)
{
- int wndx;
-
- for (wndx = 0; wndx < wqueue->nthreads; wndx++)
- {
- if (wqueue->worker[wndx].work == work &&
- wqueue->worker[wndx].pid != nxsched_gettid())
Review Comment:
why remove this block
--
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
