Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 merged PR #16231: URL: https://github.com/apache/nuttx/pull/16231 -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on PR #16231: URL: https://github.com/apache/nuttx/pull/16231#issuecomment-2854034824 @Fix-Point please fix: ``` /home/runner/work/nuttx/nuttx/nuttx/sched/wdog/wd_start.c:297: Absoulute ==> Absolute `` -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231: URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074918858 ## libs/libc/wqueue/work_cancel.c: ## @@ -89,7 +89,7 @@ static int work_qcancel(FAR struct usr_wqueue_s *wqueue, */ curr = wqueue->q.head; - while (curr && curr != &work->u.s.dq) + while (curr && curr != (FAR dq_entry_t *)&work->node) Review Comment: Done. ## libs/libc/wqueue/work_queue.c: ## @@ -87,17 +87,17 @@ static int work_qqueue(FAR struct usr_wqueue_s *wqueue, /* Initialize the work structure */ - 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->worker = worker; /* Work callback. non-NULL means queued */ + work->arg= arg; /* Callback argument */ + work->qtime = clock() + delay; /* Delay until work performed */ /* Do the easy case first -- when the work queue is empty. */ if (wqueue->q.head == NULL) Review Comment: Done. ## libs/libc/wqueue/work_usrthread.c: ## @@ -136,7 +136,7 @@ static void work_process(FAR struct usr_wqueue_s *wqueue) dq_remfirst(&wqueue->q); Review Comment: Done. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231: URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074762646 ## libs/libc/wqueue/work_queue.c: ## @@ -87,17 +87,17 @@ static int work_qqueue(FAR struct usr_wqueue_s *wqueue, /* Initialize the work structure */ - 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->worker = worker; /* Work callback. non-NULL means queued */ + work->arg= arg; /* Callback argument */ + work->qtime = clock() + delay; /* Delay until work performed */ /* Do the easy case first -- when the work queue is empty. */ if (wqueue->q.head == NULL) Review Comment: need change from dq_entry_s to list_node in usr_wqueue_s at: https://github.com/apache/nuttx/blob/master/libs/libc/wqueue/wqueue.h#L52 ## libs/libc/wqueue/work_usrthread.c: ## @@ -136,7 +136,7 @@ static void work_process(FAR struct usr_wqueue_s *wqueue) dq_remfirst(&wqueue->q); Review Comment: need change all dq_xxx to list_xxx in this file -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231: URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074681300 ## libs/libc/wqueue/work_cancel.c: ## @@ -89,7 +89,7 @@ static int work_qcancel(FAR struct usr_wqueue_s *wqueue, */ curr = wqueue->q.head; - while (curr && curr != &work->u.s.dq) + while (curr && curr != (FAR dq_entry_t *)&work->node) Review Comment: let's use list_node for usr_wqueue_s in the first patch too -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074680567
##
sched/wqueue/kwork_thread.c:
##
@@ -110,6 +116,53 @@ struct lp_wqueue_s g_lpwork =
* Private Functions
/
+static inline_function
+void work_dispatch(FAR struct kwork_wqueue_s *wq)
+{
+ FAR struct work_s *work;
+ FAR struct work_s *next;
+ unsigned int count = 0;
Review Comment:
let's remove the count and call nxsem_post in the loop directly
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074680361
##
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:
Ok.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074674647
##
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:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074673094
##
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:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074669532
##
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:
`list_is_head(list, item)` will be expanded to `((list)->next == (item))`.
The work node will not be accessed. We are not supposed to access
`wqueue->pending.next` since it is already cold.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074646362
##
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:
If we moved this `nxsem_post` to the `work_dispatch`, we have to set a timer
that immediately fires, which is less-efficient. Here we are doing what the
timer callback should do to wake up at least 1 worker thread.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074646468
##
sched/wqueue/kwork_queue.c:
##
@@ -141,65 +80,72 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
FAR void *arg, clock_t delay, clock_t period)
{
irqstate_t flags;
- int ret = OK;
+ clock_texpected;
+ bool wake = false;
+ int ret = OK;
if (wqueue == NULL || work == NULL || worker == NULL)
{
return -EINVAL;
}
+ /* delay+1 is to prevent the insufficient sleep time if we are
+ * currently near the boundary to the next tick.
+ * | current_tick | current_tick + 1 | current_tick + 2 | |
+ * | ^ Here we get the current tick
+ * In this case we delay 1 tick, timer will be triggered at
+ * current_tick + 1, which is not enough for at least 1 tick.
+ */
+
+ expected = clock_systime_ticks() + delay + 1;
+
/* Interrupts are disabled so that this logic can be called from with
* task logic or from interrupt handling logic.
*/
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)
+ 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).
- */
-
- work->worker = NULL;
- wd_cancel(&work->u.timer);
-
- list_delete(&work->u.s.node);
-}
+ /* Seize the ownership from the work thread. */
- if (work_is_canceling(wqueue->worker, wqueue->nthreads, work))
-{
- goto out;
+ list_delete(&work->node);
Review Comment:
Done.
##
sched/wqueue/kwork_queue.c:
##
@@ -141,65 +80,72 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
FAR void *arg, clock_t delay, clock_t period)
{
irqstate_t flags;
- int ret = OK;
+ clock_texpected;
+ bool wake = false;
+ int ret = OK;
if (wqueue == NULL || work == NULL || worker == NULL)
{
return -EINVAL;
}
+ /* delay+1 is to prevent the insufficient sleep time if we are
+ * currently near the boundary to the next tick.
+ * | current_tick | current_tick + 1 | current_tick + 2 | |
+ * | ^ Here we get the current tick
+ * In this case we delay 1 tick, timer will be triggered at
+ * current_tick + 1, which is not enough for at least 1 tick.
+ */
+
+ expected = clock_systime_ticks() + delay + 1;
+
/* Interrupts are disabled so that this logic can be called from with
* task logic or from interrupt handling logic.
*/
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)
+ 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).
- */
-
- work->worker = NULL;
- wd_cancel(&work->u.timer);
-
- list_delete(&work->u.s.node);
-}
+ /* Seize the ownership from the work thread. */
- if (work_is_canceling(wqueue->worker, wqueue->nthreads, work))
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074631788
##
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);
}
- /* Extract the work description from the entry (in case the work
- * instance will be re-used after it has been de-queued).
+ flags = spin_lock_irqsave(&wqueue->lock);
+ sched_lock();
+}
+
+ if (!list_is_empty(&wqueue->expired))
+{
+ work = list_first_entry(&wqueue->expired, struct work_s, node);
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074631337
##
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:
We can check if the `curr_node` is the list, which is cache-friendly and can
reduce one memory access comparing to call `list_is_head(&wqueue->pending,
&work->node)`.
##
sched/wqueue/kwork_cancel.c:
##
@@ -89,6 +109,7 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
bool sync,
}
spin_unlock_irqrestore(&wqueue->lock, flags);
+
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074628950
##
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;
- worker_t worker; /* Work callback */
- FAR void *arg; /* Callback argument */
- FAR struct kwork_wqueue_s *wq; /* Work queue */
+ struct list_node node; /* Implements a double linked list */
Review Comment:
Done.
##
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.
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074630963
##
sched/wqueue/kwork_queue.c:
##
@@ -141,65 +80,72 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
FAR void *arg, clock_t delay, clock_t period)
{
irqstate_t flags;
- int ret = OK;
+ clock_texpected;
+ bool wake = false;
+ int ret = OK;
if (wqueue == NULL || work == NULL || worker == NULL)
{
return -EINVAL;
}
+ /* delay+1 is to prevent the insufficient sleep time if we are
+ * currently near the boundary to the next tick.
+ * | current_tick | current_tick + 1 | current_tick + 2 | |
+ * | ^ Here we get the current tick
+ * In this case we delay 1 tick, timer will be triggered at
+ * current_tick + 1, which is not enough for at least 1 tick.
+ */
+
+ expected = clock_systime_ticks() + delay + 1;
+
/* Interrupts are disabled so that this logic can be called from with
* task logic or from interrupt handling logic.
*/
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)
+ 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).
- */
-
- work->worker = NULL;
- wd_cancel(&work->u.timer);
-
- list_delete(&work->u.s.node);
-}
+ /* Seize the ownership from the work thread. */
- if (work_is_canceling(wqueue->worker, wqueue->nthreads, work))
-{
- goto out;
+ list_delete(&work->node);
Review Comment:
We can check if the `curr_node` is the list, which is cache-friendly and can
reduce one memory access comparing to call `list_is_head(&wqueue->pending,
&work->node)`.
##
sched/wqueue/kwork_queue.c:
##
@@ -141,65 +80,72 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
FAR void *arg, clock_t delay, clock_t period)
{
irqstate_t flags;
- int ret = OK;
+ clock_texpected;
+ bool wake = false;
+ int ret = OK;
if (wqueue == NULL || work == NULL || worker == NULL)
{
return -EINVAL;
}
+ /* delay+1 is to prevent the insufficient sleep time if we are
+ * currently near the boundary to the next tick.
+ * | current_tick | current_tick + 1 | current_tick + 2 | |
+ * | ^ Here we get the current tick
+ * In this case we delay 1 tick, timer will be triggered at
+ * current_tick + 1, which is not enough for at least 1 tick.
+ */
+
+ expected = clock_systime_ticks() + delay + 1;
+
/* Interrupts are disabled so that this logic can be called from with
* task logic or from interrupt handling logic.
*/
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)
+ 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).
- */
-
- work->worker = NULL;
- wd_cancel(&work->u.timer);
-
- list_delete(&work->u.s.node);
-}
+ /* Seize the ownership from the work thread. */
- if (work_is_canceling(wqueue->worker, wqueue->nthreads, work))
-{
- goto out;
+ list_delete(&work->node);
Review Comment:
We can check if the `curr_node` is the list, which is cache-friendly and can
reduce one memory access comparing to call `list_is_head(&wqueue->pending,
&work->node)`.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2074628825
##
sched/wqueue/kwork_cancel.c:
##
@@ -31,7 +31,7 @@
#include
#include
-#include
+#include
Review Comment:
Done.
##
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_tlock; /* 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 expired; /* The queue of expired work. */
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2072976590
##
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_tlock; /* 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 expired; /* The queue of expired work. */
Review Comment:
remove the extra spaces
##
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.
Review Comment:
work->qtime
##
sched/wqueue/kwork_cancel.c:
##
@@ -89,6 +109,7 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
bool sync,
}
spin_unlock_irqrestore(&wqueue->lock, flags);
+
Review Comment:
revert the change
##
sched/wqueue/kwork_queue.c:
##
@@ -141,65 +80,72 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
FAR void *arg, clock_t delay, clock_t period)
{
irqstate_t flags;
- int ret = OK;
+ clock_texpected;
+ bool wake = false;
+ int ret = OK;
if (wqueue == NULL || work == NULL || worker == NULL)
{
return -EINVAL;
}
+ /* delay+1 is to prevent the insufficient sleep time if we are
+ * currently near the boundary to the next tick.
+ * | current_tick | current_tick + 1 | current_tick + 2 | |
+ * | ^ Here we get the current tick
+ * In this case we delay 1 tick, timer will be triggered at
+ * current_tick + 1, which is not enough for at least 1 tick.
+ */
+
+ expected = clock_systime_ticks() + delay + 1;
+
/* Interrupts are disabled so that this logic can be called from with
* task logic or from interrupt handling logic.
*/
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)
+ 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).
- */
-
- work->worker = NULL;
- wd_cancel(&work->u.timer);
-
- list_delete(&work->u.s.node);
-}
+ /* Seize the ownership from the work thread. */
- if (work_is_canceling(wqueue->worker, wqueue->nthreads, work))
Review Comment:
why remove this check
##
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_l
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
fdcavalcanti commented on PR #16231: URL: https://github.com/apache/nuttx/pull/16231#issuecomment-2845214664 I've found no issues on Espressif's CI test results. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
fdcavalcanti commented on PR #16231: URL: https://github.com/apache/nuttx/pull/16231#issuecomment-2841971496 Most tests look ok, however `sotest` defconfig is failing to build across many SoCs due to some LIBC errors, which I suspect is due to the apps directory not in sync. Some other apps are failing on the `refresh.sh` script. Can you rebase this change? That would give us better results. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
fdcavalcanti commented on PR #16231: URL: https://github.com/apache/nuttx/pull/16231#issuecomment-2841709726 I'll run this through Espressif's CI and see if anything comes up. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2065351185
##
sched/wqueue/kwork_queue.c:
##
@@ -141,65 +80,50 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
FAR void *arg, clock_t delay, clock_t period)
{
irqstate_t flags;
+ clock_texpected;
int ret = OK;
if (wqueue == NULL || work == NULL || worker == NULL)
{
return -EINVAL;
}
+ if (!delay)
+{
+ expected = clock_systime_ticks();
+}
+ else
+{
+ expected = clock_systime_ticks() + delay + 1;
+}
+
/* Interrupts are disabled so that this logic can be called from with
* task logic or from interrupt handling logic.
*/
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)
+ 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. */
- 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);
-}
-}
-
- 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->worker = worker; /* Work callback. non-NULL means queued */
+ work->arg= arg; /* Callback argument */
+ work->qtime = expected; /* Expected time */
+ work->period = period; /* Periodical delay */
- /* Queue the new work */
+ /* Insert to the pending list of the wqueue. */
- if (!delay)
-{
- queue_work(wqueue, work);
-}
- else if (period == 0)
-{
- ret = wd_start(&work->u.timer, delay,
- work_timer_expiry, (wdparm_t)work);
-}
- else
-{
- ret = wd_start_period(&work->u.ptimer, delay, period,
-work_timer_expiry, (wdparm_t)work);
-}
+ work_insert_pending(wqueue, work);
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
GUIDINGLI commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2063219741
##
sched/wqueue/kwork_queue.c:
##
@@ -141,65 +80,50 @@ int work_queue_period_wq(FAR struct kwork_wqueue_s *wqueue,
FAR void *arg, clock_t delay, clock_t period)
{
irqstate_t flags;
+ clock_texpected;
int ret = OK;
if (wqueue == NULL || work == NULL || worker == NULL)
{
return -EINVAL;
}
+ if (!delay)
+{
+ expected = clock_systime_ticks();
+}
+ else
+{
+ expected = clock_systime_ticks() + delay + 1;
+}
+
/* Interrupts are disabled so that this logic can be called from with
* task logic or from interrupt handling logic.
*/
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)
+ 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. */
- 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);
-}
-}
-
- 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->worker = worker; /* Work callback. non-NULL means queued */
+ work->arg= arg; /* Callback argument */
+ work->qtime = expected; /* Expected time */
+ work->period = period; /* Periodical delay */
- /* Queue the new work */
+ /* Insert to the pending list of the wqueue. */
- if (!delay)
-{
- queue_work(wqueue, work);
-}
- else if (period == 0)
-{
- ret = wd_start(&work->u.timer, delay,
- work_timer_expiry, (wdparm_t)work);
-}
- else
-{
- ret = wd_start_period(&work->u.ptimer, delay, period,
-work_timer_expiry, (wdparm_t)work);
-}
+ work_insert_pending(wqueue, work);
Review Comment:
if the delay is zero, then directly add to ready list ?
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
GUIDINGLI commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2062565038
##
sched/wqueue/wqueue.h:
##
@@ -66,14 +66,15 @@ struct kworker_s
struct kwork_wqueue_s
{
- struct dq_queue_s q; /* The queue of pending work */
- sem_t sem; /* The counting semaphore of the wqueue */
- sem_t exsem; /* Sync waiting for thread exit */
- spinlock_tlock; /* 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 ready; /* The queue of ready work. */
Review Comment:
so, can we use one list ?
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2057799243
##
sched/wqueue/kwork_thread.c:
##
@@ -216,17 +278,54 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
- wqueue->wait_count++;
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->pending))
+{
+ work = list_first_entry(&wqueue->pending, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
Review Comment:
The semantic of `ticks` has changed if `has_next == true`. It represents the
next expired time.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
GUIDINGLI commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2055620048
##
sched/wqueue/kwork_thread.c:
##
@@ -216,17 +278,54 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
- wqueue->wait_count++;
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->pending))
+{
+ work = list_first_entry(&wqueue->pending, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
Review Comment:
the ticks base time is clock_systime_ticks() in line 208, and here must be
has a continue ?
##
sched/wqueue/kwork_thread.c:
##
@@ -216,18 +288,49 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->q))
+{
+ work = list_first_entry(&wqueue->q, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
wqueue->wait_count++;
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
+{
+ /* Continue the work thread loop. */
+
+ continue;
+}
+
+ /* Else we start a time to wake up the work thread. */
+
+ wd_start_abstick(&wqueue->timer, ticks, work_timer_expired,
Review Comment:
how about the NTHREDS > 1, and the list only has one, so the each thread
will setup their own wdog(but the same value) to wake thread
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051878926
##
sched/wqueue/kwork_thread.c:
##
@@ -216,18 +288,49 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->q))
+{
+ work = list_first_entry(&wqueue->q, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
wqueue->wait_count++;
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
+{
+ /* Continue the work thread loop. */
+
+ continue;
+}
+
+ /* Else we start a time to wake up the work thread. */
+
+ wd_start_abstick(&wqueue->timer, ticks, work_timer_expired,
Review Comment:
I will try to optimize the synchronization in the next patch.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051878460
##
sched/wqueue/kwork_cancel.c:
##
@@ -59,40 +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);
- if (dq_inqueue((FAR dq_entry_t *)work, &wqueue->q))
-{
- dq_rem((FAR dq_entry_t *)work, &wqueue->q);
-}
- ret = OK;
+ list_delete(&work->node);
}
- else if (!up_interrupt_context() && !sched_idletask() && sync)
+
+ spin_unlock_irqrestore(&wqueue->lock, flags);
+
+ if (run_myself)
Review Comment:
Fixed.
##
sched/wqueue/kwork_queue.c:
##
@@ -155,51 +94,46 @@ 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)
+ 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. */
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);
-}
-}
- 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();
}
- else if (period == 0)
+ else
{
- ret = wd_start(&work->u.timer, delay,
- work_timer_expiry, (wdparm_t)work);
+ work->qtime = clock_systime_ticks() + delay + 1;
}
- else
+
+ /* Insert to the workqueue's waiting list. */
+
+ work_insert_queue(wqueue, work);
+
+ /* Wakeup the wqueue thread. */
+
+ if (wqueue->wait_count > 0) /* There are threads waiting for sem. */
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
anchao commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051672835
##
sched/wqueue/kwork_thread.c:
##
@@ -216,18 +288,49 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->q))
+{
+ work = list_first_entry(&wqueue->q, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
wqueue->wait_count++;
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
+{
+ /* Continue the work thread loop. */
+
+ continue;
+}
+
+ /* Else we start a time to wake up the work thread. */
+
+ wd_start_abstick(&wqueue->timer, ticks, work_timer_expired,
Review Comment:
We should optimize the current wait strategy and wait only when there are no
enqueue works or the timer is started, so that `wait_count` and `sem_post` in
this function should be removed
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
anchao commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051672996
##
sched/wqueue/kwork_thread.c:
##
@@ -216,18 +288,49 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->q))
+{
+ work = list_first_entry(&wqueue->q, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
wqueue->wait_count++;
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
+{
+ /* Continue the work thread loop. */
+
+ continue;
+}
+
+ /* Else we start a time to wake up the work thread. */
+
+ wd_start_abstick(&wqueue->timer, ticks, work_timer_expired,
Review Comment:
> We should optimize the current wait strategy and wait only when there are
no enqueue works or the timer is started, so that `wait_count` and `sem_post`
in this function should be removed
##
sched/wqueue/kwork_thread.c:
##
@@ -216,18 +288,49 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->q))
+{
+ work = list_first_entry(&wqueue->q, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
wqueue->wait_count++;
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
+{
+ /* Continue the work thread loop. */
+
+ continue;
+}
+
+ /* Else we start a time to wake up the work thread. */
+
+ wd_start_abstick(&wqueue->timer, ticks, work_timer_expired,
Review Comment:
> We should optimize the current wait strategy and wait only when there are
no enqueue works or the timer is started, so that `wait_count` and `sem_post`
in this function should be removed
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
anchao commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051672835
##
sched/wqueue/kwork_thread.c:
##
@@ -216,18 +288,49 @@ static int work_thread(int argc, FAR char *argv[])
* posted.
*/
+ /* Check the waiting queue */
+
+ has_next = false;
+
+ if (!list_is_empty(&wqueue->q))
+{
+ work = list_first_entry(&wqueue->q, struct work_s, node);
+
+ /* The work thread will go sleep until work->qtime */
+
+ ticks = work->qtime;
+ has_next = true;
+}
+
wqueue->wait_count++;
spin_unlock_irqrestore(&wqueue->lock, flags);
sched_unlock();
+ /* Set the earliest expired work delay */
+
+ if (has_next)
+{
+ /* If the earliest work has already expired. */
+
+ if (clock_compare(ticks, clock_systime_ticks()))
+{
+ /* Continue the work thread loop. */
+
+ continue;
+}
+
+ /* Else we start a time to wake up the work thread. */
+
+ wd_start_abstick(&wqueue->timer, ticks, work_timer_expired,
Review Comment:
We should optimize the current wait strategy and wait only when there are no
enqueue works or the timer is started, so that `wait_count` and `sem_post` in
this code should be removed
##
sched/wqueue/kwork_queue.c:
##
@@ -155,51 +94,46 @@ 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)
+ 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. */
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);
-}
-}
- 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();
}
- else if (period == 0)
+ else
{
- ret = wd_start(&work->u.timer, delay,
- work_timer_expiry, (wdparm_t)work);
+ work->qtime = clock_systime_ticks() + delay + 1;
}
- else
+
+ /* Insert to the workqueue's waiting list. */
+
+ work_insert_queue(wqueue, work);
+
+ /* Wakeup the wqueue thread. */
+
+ if (wqueue->wait_count > 0) /* There are threads waiting for sem. */
Review Comment:
no need to wake up the worker thread if the work does not expiration
##
sched/wqueue/kwork_cancel.c:
##
@@ -59,40 +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);
- if (dq_inqueue((FAR dq_entry_t *)work, &wqueue->q))
-{
- dq_rem((FAR dq_entry_t *)work, &wqueue->q);
-}
- ret = OK;
+ list_delete(&work->node);
}
- else if (!up_interrupt_context() && !sched_idletask() && sync)
+
+ spin_unlock_irqrestore(&wqueue->lock, flags);
+
+ if (run_myself)
Review Comment:
This is wrong. **cancel_sync** means waiting for the task being executed to
end, rather than executing the call in the current task context. Such an
implementation could easily cause deadlock.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051427658
##
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:
but how to wait until the callback finish in work_cancel_sync?
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051350753
##
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:
In next patch I'll try to optimize the synchronization.
##
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:
OK. I'll create a new patch after this.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051350399
##
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:
This block is not required anymore since the cancel thread will complete the
unfinished work by itself.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051342956
##
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:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051342772
##
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:
In both local list or shared list, we can use the `list_delete` in
critical-section to remove the work safely.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2051342390
##
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_tlock; /* 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:
Done.
##
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:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049951035
##
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:
Inline callback is removed.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049963827
##
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:
Fixed the cancel.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
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_tlock; /* 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050366248
##
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_tlock; /* 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:
Done
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050365050
##
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:
Done
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +148,32 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231: URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050363865 ## sched/wqueue/wqueue.h: ## @@ -159,6 +148,32 @@ static inline_function FAR struct kwork_wqueue_s *work_qid2wq(int qid) } } +static inline_function +void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue, + struct work_s *work) Review Comment: Done. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050363865
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +148,32 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
Review Comment:
Done.
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +148,32 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
+{
+ struct work_s *curr;
Review Comment:
Done.
##
sched/wqueue/kwork_thread.c:
##
@@ -135,11 +181,14 @@ struct lp_wqueue_s g_lpwork =
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;
+ FAR struct kworker_s *kworker;
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050352585
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +148,32 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
+{
+ struct work_s *curr;
Review Comment:
ditto
##
sched/wqueue/kwork_thread.c:
##
@@ -135,11 +181,14 @@ struct lp_wqueue_s g_lpwork =
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;
+ FAR struct kworker_s *kworker;
Review Comment:
remove the extra spaces
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +148,32 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
Review Comment:
add FAR
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050351112
##
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:
Ok
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050349735
##
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:
but no change
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
anchao commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049923533
##
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:
The current version does not implement this feature. so has this commit not
been reviewed by you? I thought the intention was to remove the sync semantics.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050107421
##
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:
Fixed.
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +149,24 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
+{
+ struct list_node *curr;
+
+ list_for_every(&wqueue->wait_q, curr)
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050007793
##
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:
> The current version does not implement this feature. so has this commit
not been reviewed by you? I thought the intention was to remove the sync
semantics.
it's a hot fix, not go through the internal review first.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050007021
##
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:
no, you can't remove the sync version, since the caller need sync to avoid
the race condition.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
xiaoxiang781216 commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2050007021
##
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:
no, you can't remove the sync version, since the race condition will happen.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049938581
##
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:
Some modules call the `work_cancel_sync()`. Remove it lead to compilation
error. I am going to remove the API in the next patch.
##
sched/wqueue/kwork_queue.c:
##
@@ -155,51 +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)
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049939836
##
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:
Done.
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +149,24 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
+{
+ struct list_node *curr;
+
+ list_for_every(&wqueue->wait_q, curr)
+{
+ struct work_s *curr_work = list_entry(curr, struct work_s, node);
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049947713
##
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:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049958705
##
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:
fixed
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049951335
##
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:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049949011
##
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:
I will try this in the next patch.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049948161
##
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:
Done.
##
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:
Done.
##
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_tlock; /* 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:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231: URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049939697 ## 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: Fixed. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049939572
##
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)
+{
+ struct work_s *work;
+
+ while (!list_is_empty(&wq->wait_q))
Review Comment:
Done.
##
sched/wqueue/kwork_thread.c:
##
@@ -181,7 +234,31 @@ static int work_thread(int argc, FAR char *argv[])
/* Mark the work as no longer being queued */
- work->worker = NULL;
+ if (work->period != 0)
+{
+ /* Insert qtime */
+
+ work->qtime += work->period;
+
+ if (clock_compare(work->qtime, ticks))
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
Fix-Point commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049938678
##
sched/wqueue/kwork_queue.c:
##
@@ -155,51 +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);
- if (dq_inqueue((FAR dq_entry_t *)work, &wqueue->q))
-{
- dq_rem((FAR dq_entry_t *)work, &wqueue->q);
-}
-}
- 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);
sched_unlock();
return ret;
Review Comment:
Done.
--
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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
anchao commented on code in PR #16231: URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049926527 ## 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: The readytorun queue is now only used in this function. If the member is not accessed by the external API in the design, it needs to be privatized in this function. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
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_tlock; /* 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 l
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
anchao commented on code in PR #16231: URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049169728 ## 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: maybe wait_q have ready worker now -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
anchao commented on code in PR #16231:
URL: https://github.com/apache/nuttx/pull/16231#discussion_r2049150464
##
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:
```suggestion
static inline_function void work_timer_expired(wdparm_t arg)
```
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +149,24 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+static inline_function
+void work_insert_waitq(FAR struct kwork_wqueue_s *wqueue,
+ struct work_s *work)
+{
+ struct list_node *curr;
+
+ list_for_every(&wqueue->wait_q, curr)
Review Comment:
list_for_every_entry
##
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:
move readytorun queue into stack
##
sched/wqueue/kwork_queue.c:
##
@@ -155,51 +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);
- if (dq_inqueue((FAR dq_entry_t *)work, &wqueue->q))
-{
- dq_rem((FAR dq_entry_t *)work, &wqueue->q);
-}
-}
- 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);
sched_unlock();
return ret;
Review Comment:
```suggestion
return 0;
```
##
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)
+{
+ struct work_s *work;
+
+ while (!list_is_empty(&wq->wait_q))
Review Comment:
list_for_every_entry
##
sched/wqueue/kwork_thread.c:
##
@@ -181,7 +234,31 @@ static int work_thread(int argc, FAR char *argv[])
/* Mark the work as no longer being queued */
- work->worker = NULL;
+ if (work->period != 0)
+{
+ /* Insert qtime */
+
+ work->qtime += work->period;
+
+ if (clock_compare(work->qtime, ticks))
Review Comment:
ticks should be updated again after each polling
##
sched/wqueue/wqueue.h:
##
@@ -159,6 +149,24 @@ static inline_function FAR struct kwork_wqueue_s
*work_qid2wq(int qid)
}
}
+s
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
acassis commented on PR #16231: URL: https://github.com/apache/nuttx/pull/16231#issuecomment-2813291589 @Fix-Point @xiaoxiang781216 I think it should be tested on real hardware as well. Normally small changes on core features like this could break the system. I think it requires test on sensors (APDS9960) and devices that uses workqueue. -- 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
Re: [PR] sched/wqueue: Refactor delayed and periodic workqueue. [nuttx]
nuttxpr commented on PR #16231: URL: https://github.com/apache/nuttx/pull/16231#issuecomment-2812005286 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR summary is good, clearly explaining the "why" and "how". However, it lacks crucial cross-referencing to a NuttX issue. This is mandatory. The submitter *must* create an issue first and link the PR to it. The Impact section is too vague. While it mentions affected modules, it doesn't detail the *specific* impacts as requested by the template. The template explicitly asks for YES/NO answers for each category with descriptions. The author needs to address each point individually (Impact on user, build, hardware, documentation, security, compatibility) with YES/NO and explanations. For example, even if the impact on the user is minimal, they should state "NO" followed by a brief justification like "Existing user code using workqueues should continue to function as expected with potentially slightly different timing." The Testing section is insufficient. While it mentions the test used (`wqueue_test`), it lacks vital information. The template *requires* logs before *and* after the change. Simply stating "Periodical workqueue can work now" is not enough. Concrete evidence of improvement or correct functionality is necessary through logs. The Build Host details are also missing (OS, compiler version, etc.). Furthermore, testing on only one platform (QEMU/x86_64) is likely insufficient for a core change like this. More architectures and boards should be tested. **Therefore, this PR does *not* fully meet the NuttX requirements.** The author needs to: 1. **Link to a NuttX Issue:** Create a corresponding issue in the NuttX issue tracker and reference it in the PR summary. 2. **Elaborate on Impact:** Provide specific YES/NO answers for each impact category with detailed explanations. 3. **Improve Testing:** Include "before" and "after" logs, specify the build host environment details, and ideally test on more platforms/architectures. By addressing these points, the PR will be much stronger and have a higher chance of being accepted. -- 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
