xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049854411
##########
include/nuttx/wqueue.h:
##########
@@ -249,19 +249,11 @@ typedef CODE void (*worker_t)(FAR void *arg);
struct work_s
{
- union
- {
- struct
- {
- struct list_node node; /* Implements a double linked list */
- clock_t qtime; /* Time work queued */
- } s;
- struct wdog_s timer; /* Delay expiry timer */
- struct wdog_period_s ptimer; /* Period expiry timer */
- } u;
+ struct list_node node; /* Implements a double linked list */
+ clock_t qtime; /* Time work queued */
+ clock_t period; /* Periodical delay ticks */
Review Comment:
remove the extra spaces before comment
##########
sched/wqueue/kwork_thread.c:
##########
@@ -83,11 +83,15 @@
struct hp_wqueue_s g_hpwork =
{
- LIST_INITIAL_VALUE(g_hpwork.q),
- SEM_INITIALIZER(0),
- SEM_INITIALIZER(0),
- SP_UNLOCKED,
- CONFIG_SCHED_HPNTHREADS,
+ .wq =
Review Comment:
don't use .xxx = yyy, which some old compiler doesn't support c99 feature.
##########
sched/wqueue/kwork_thread.c:
##########
@@ -110,6 +118,44 @@ struct lp_wqueue_s g_lpwork =
* Private Functions
****************************************************************************/
+static inline void work_timer_expired(wdparm_t arg)
Review Comment:
why add inline which is always called through pointer
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -59,38 +59,20 @@ 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->worker != NULL)
{
- /* 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. */
work->worker = NULL;
- wd_cancel(&work->u.timer);
- if (dq_inqueue((FAR dq_entry_t *)work, &wqueue->q))
- {
- dq_rem((FAR dq_entry_t *)work, &wqueue->q);
- }
- ret = OK;
- }
- else if (!up_interrupt_context() && !sched_idletask() && sync)
- {
- int wndx;
-
- for (wndx = 0; wndx < wqueue->nthreads; wndx++)
- {
- if (wqueue->worker[wndx].work == work &&
- wqueue->worker[wndx].pid != nxsched_gettid())
- {
- wqueue->worker[wndx].wait_count++;
- spin_unlock_irqrestore(&wqueue->lock, flags);
- nxsem_wait_uninterruptible(&wqueue->worker[wndx].wait);
- return 1;
- }
- }
+ list_delete(&work->node);
}
+ ret = OK;
Review Comment:
how to handle the caller want to wait until the callback finish?
##########
libs/libc/wqueue/work_queue.c:
##########
@@ -89,7 +89,7 @@ static int work_qqueue(FAR struct usr_wqueue_s *wqueue,
work->worker = worker; /* Work callback. non-NULL means queued */
work->arg = arg; /* Callback argument */
- work->u.s.qtime = clock() + delay; /* Delay until work performed */
+ work->qtime = clock() + delay; /* Delay until work performed */
Review Comment:
remove two spaces before comment
##########
sched/wqueue/wqueue.h:
##########
@@ -104,13 +100,7 @@ struct hp_wqueue_s
#ifdef CONFIG_SCHED_LPWORK
struct lp_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 kwork_wqueue_s wq;
Review Comment:
ditto
##########
sched/wqueue/kwork_thread.c:
##########
@@ -160,14 +208,19 @@ static int work_thread(int argc, FAR char *argv[])
* so ourselves, and (2) there will be no changes to the work queue
*/
+ ticks = clock_systime_ticks();
+
+ /* Check if there is expired delayed work */
+
+ work_expiration(ticks, wqueue);
+
/* Remove the ready-to-execute work from the list */
- while ((work = (FAR struct work_s *)dq_remfirst(&wqueue->q)) != NULL)
+ while (!list_is_empty(&wqueue->q))
Review Comment:
what's benefit? this suggestion require the change to add another loop to
handle the work item maybe add after line 198
##########
sched/wqueue/kwork_thread.c:
##########
@@ -211,6 +288,17 @@ static int work_thread(int argc, FAR char *argv[])
}
}
+ if (!list_is_empty(&wqueue->wait_q))
Review Comment:
don't need call wd_start_abstick again if the head of wait_q isn't changed
##########
sched/wqueue/kwork_thread.c:
##########
@@ -110,6 +118,44 @@ struct lp_wqueue_s g_lpwork =
* Private Functions
****************************************************************************/
+static inline 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)
+ {
+ wq->wait_count--;
+ nxsem_post(&wq->sem);
+ }
+
+ spin_unlock_irqrestore(&wq->lock, flags);
+ sched_unlock();
+}
+
+static inline
+void work_expiration(clock_t ticks, FAR struct kwork_wqueue_s *wq)
Review Comment:
void work_expiration(FAR struct kwork_wqueue_s *wq, clock_t ticks)
##########
sched/wqueue/kwork_queue.c:
##########
@@ -155,49 +94,48 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
flags = spin_lock_irqsave(&wqueue->lock);
sched_lock();
- /* Remove the entry from the timer and work queue. */
+ /* Check whether we own the work structure. */
if (work->worker != NULL)
{
- /* 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. */
work->worker = NULL;
- wd_cancel(&work->u.timer);
- list_delete(&work->u.s.node);
- }
-
- if (work_is_canceling(wqueue->worker, wqueue->nthreads, work))
- {
- goto out;
+ list_delete(&work->node);
}
/* Initialize the work structure. */
work->worker = worker; /* Work callback. non-NULL means queued */
work->arg = arg; /* Callback argument */
- work->wq = wqueue; /* Work queue */
+ work->period = period; /* Periodical delay */
/* Queue the new work */
if (!delay)
{
- queue_work(wqueue, work);
+ work->qtime = clock_systime_ticks();
+
+ list_add_tail(&wqueue->q, &work->node);
}
- else if (period == 0)
+ else
{
- ret = wd_start(&work->u.timer, delay,
- work_timer_expiry, (wdparm_t)work);
+ /* Insert the work to the workqueue's waiting list. */
+
+ work->qtime = clock_systime_ticks() + delay + 1;
+
+ work_insert_waitq(wqueue, work);
}
- else
+
+ /* Wakeup the wqueue thread. */
+
+ if (wqueue->wait_count > 0) /* There are threads waiting for sem. */
{
- ret = wd_start_period(&work->u.ptimer, delay, period,
- work_timer_expiry, (wdparm_t)work);
+ wqueue->wait_count--;
+ nxsem_post(&wqueue->sem);
}
-out:
spin_unlock_irqrestore(&wqueue->lock, flags);
Review Comment:
could we move spin_unlock_irqrestore before line 136? so, we can remove
sched_lock/sched_unlock
##########
sched/wqueue/wqueue.h:
##########
@@ -83,13 +85,7 @@ struct kwork_wqueue_s
#ifdef CONFIG_SCHED_HPWORK
struct hp_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 kwork_wqueue_s wq;
Review Comment:
remove or align worker at line 92
##########
include/nuttx/wqueue.h:
##########
@@ -249,19 +249,11 @@ typedef CODE void (*worker_t)(FAR void *arg);
struct work_s
{
- union
- {
- struct
- {
- struct list_node node; /* Implements a double linked list */
- clock_t qtime; /* Time work queued */
- } s;
- struct wdog_s timer; /* Delay expiry timer */
- struct wdog_period_s ptimer; /* Period expiry timer */
- } u;
+ struct list_node node; /* Implements a double linked list */
Review Comment:
remove one space before node
##########
sched/wqueue/kwork_thread.c:
##########
@@ -110,6 +118,44 @@ struct lp_wqueue_s g_lpwork =
* Private Functions
****************************************************************************/
+static inline 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)
+ {
+ wq->wait_count--;
+ nxsem_post(&wq->sem);
+ }
+
+ spin_unlock_irqrestore(&wq->lock, flags);
Review Comment:
move before line 130 to remove sched_lock/sched_unlock
--
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
