You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/09/06 13:47:59 UTC
[GitHub] [incubator-nuttx] GUIDINGLI opened a new pull request, #7018: wqueue: change single queue to double queue to improve speed
GUIDINGLI opened a new pull request, #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018
## Summary
wqueue: change single queue to double queue to improve speed
wqueue: fix work_qcancel() judge error caused by the union in struct work_s
in struct work_s:
union
{
struct
{
struct sq_entry_s sq; /* Implements a single linked list */
clock_t qtime; /* Time work queued */
} s;
struct wdog_s timer; /* Delay expiry timer */
}
while we use WDOG_ISACTIVE(&work->timer) to decide use sq or timer,
that is error, wd_cancel() maybe modify this area, sq_rem() also can
modify this area.
So we can't use the WDOG_ISACTIVE(&work->timer) to take as the judgement,
when there is a union.
Fix:
remove the union
## Impact
wqueue
## Testing
vela
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#discussion_r964470669
##########
sched/wqueue/kwork_thread.c:
##########
@@ -169,19 +168,20 @@ static int work_thread(int argc, FAR char *argv[])
/* Mark the work as no longer being queued */
work->worker = NULL;
+ leave_critical_section(flags);
/* Do the work. Re-enable interrupts while the work is being
* performed... we don't have any idea how long this will take!
*/
- leave_critical_section(flags);
CALL_WORKER(worker, arg);
- flags = enter_critical_section();
+ }
+ else
+ {
+ leave_critical_section(flags);
Review Comment:
why do we need changes in this file? I see only function reordering, but no functional changes
##########
include/nuttx/wdog.h:
##########
@@ -61,17 +61,22 @@ typedef uint32_t wdparm_t;
typedef CODE void (*wdentry_t)(wdparm_t arg);
-/* This is the internal representation of the watchdog timer structure. */
+/* This is the internal representation of the watchdog timer structure.
+ * Notice !!!
+ * Carefully with the struct wdog_s order, you may not directly modify
+ * this. This struct will combine in struct work_s in union type, and,
+ * wqueue will modify/check this struct in work_qcancel().
+ */
struct wdog_s
{
FAR struct wdog_s *next; /* Support for singly linked lists. */
+ wdparm_t arg; /* Callback argument */
Review Comment:
How ordering here bring an error in combination with union?
##########
include/nuttx/wdog.h:
##########
@@ -61,17 +61,22 @@ typedef uint32_t wdparm_t;
typedef CODE void (*wdentry_t)(wdparm_t arg);
-/* This is the internal representation of the watchdog timer structure. */
+/* This is the internal representation of the watchdog timer structure.
+ * Notice !!!
+ * Carefully with the struct wdog_s order, you may not directly modify
+ * this. This struct will combine in struct work_s in union type, and,
+ * wqueue will modify/check this struct in work_qcancel().
Review Comment:
```suggestion
* wqueue will modify/check this struct in kwork work_qcancel().
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#issuecomment-1240411762
> The question about performance is still now answered
sq -> dq,
sq_rm() -> dq_rem() On -> O1
this have a big performance improved for dq_rem() in work_qcancel().
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on a diff in pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on code in PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#discussion_r965438197
##########
sched/wqueue/kwork_thread.c:
##########
@@ -169,19 +168,20 @@ static int work_thread(int argc, FAR char *argv[])
/* Mark the work as no longer being queued */
work->worker = NULL;
+ leave_critical_section(flags);
/* Do the work. Re-enable interrupts while the work is being
* performed... we don't have any idea how long this will take!
*/
- leave_critical_section(flags);
CALL_WORKER(worker, arg);
- flags = enter_critical_section();
+ }
+ else
+ {
+ leave_critical_section(flags);
Review Comment:
Again and again, I haven't say that there is an issue.
The current code:
The restore will make sure we are back to critical_section state, but it is different to understand for a new reader.
The new code:
This is easy to understand.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on a diff in pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on code in PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#discussion_r964556230
##########
sched/wqueue/kwork_thread.c:
##########
@@ -169,19 +168,20 @@ static int work_thread(int argc, FAR char *argv[])
/* Mark the work as no longer being queued */
work->worker = NULL;
+ leave_critical_section(flags);
/* Do the work. Re-enable interrupts while the work is being
* performed... we don't have any idea how long this will take!
*/
- leave_critical_section(flags);
CALL_WORKER(worker, arg);
- flags = enter_critical_section();
+ }
+ else
+ {
+ leave_critical_section(flags);
Review Comment:
Now the order:
while (1)
{
enter_critical_section();
nxsem_wait_uninterruptible();
...
// access the work_s with critical_section
...
leave_critical_section();
}
The restore will make sure we are back to critical_section state, but it is different to understand for a new reader.
After modify:
while (1)
{
nxsem_wait_uninterruptible();
enter_critical_section();
...
// access the work_s with critical_section
...
leave_critical_section();
}
This is easy to understand.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#discussion_r965190923
##########
sched/wqueue/kwork_thread.c:
##########
@@ -169,19 +168,20 @@ static int work_thread(int argc, FAR char *argv[])
/* Mark the work as no longer being queued */
work->worker = NULL;
+ leave_critical_section(flags);
/* Do the work. Re-enable interrupts while the work is being
* performed... we don't have any idea how long this will take!
*/
- leave_critical_section(flags);
CALL_WORKER(worker, arg);
- flags = enter_critical_section();
+ }
+ else
+ {
+ leave_critical_section(flags);
Review Comment:
Again, I do not see any issue with the current approach. The existing code ensures that all code except `CALL_WORKER(worker, arg);` is executed inside the critical section, It is straight forward and easy to understand at least to me.
I will let other reviewers to write their opinion on 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#issuecomment-1240475852
done as you wish
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#issuecomment-1240114461
> The question about performance is still now answered
wd_cancel is O(n)(n is the number of active wdog) before change, now it is O(1).
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#discussion_r965191317
##########
libs/libc/queue/dq_remafter.c:
##########
@@ -0,0 +1,50 @@
+/****************************************************************************
+ * libs/libc/queue/dq_remafter.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <queue.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: dq_remafter
+ *
+ * Description:
+ * dq_remafter removes the entry following 'node' from the 'queue'.
+ * Returns a reference to the removed entry.
+ *
+ ****************************************************************************/
+
+FAR dq_entry_t *dq_remafter(FAR dq_entry_t *node, dq_queue_t *queue)
Review Comment:
```suggestion
FAR dq_entry_t *dq_remafter(FAR dq_entry_t *node, FAR dq_queue_t *queue)
```
##########
libs/libc/queue/dq_remafter.c:
##########
@@ -0,0 +1,50 @@
+/****************************************************************************
+ * libs/libc/queue/dq_remafter.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <queue.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: dq_remafter
+ *
+ * Description:
+ * dq_remafter removes the entry following 'node' from the 'queue'.
+ * Returns a reference to the removed entry.
+ *
+ ****************************************************************************/
+
+FAR dq_entry_t *dq_remafter(FAR dq_entry_t *node, dq_queue_t *queue)
+{
+ FAR dq_entry_t *ret = node->flink;
+
+ if (queue->head && ret)
Review Comment:
Optional
```suggestion
if (queue->head != NULL && ret != NULL)
```
##########
include/nuttx/wdog.h:
##########
@@ -61,17 +61,22 @@ typedef uint32_t wdparm_t;
typedef CODE void (*wdentry_t)(wdparm_t arg);
-/* This is the internal representation of the watchdog timer structure. */
+/* This is the internal representation of the watchdog timer structure.
+ * Notice !!!
+ * Carefully with the struct wdog_s order, you may not directly modify
+ * this. This struct will combine in struct work_s in union type, and,
+ * wqueue will modify/check this struct in work_qcancel().
+ */
struct wdog_s
{
FAR struct wdog_s *next; /* Support for singly linked lists. */
+ wdparm_t arg; /* Callback argument */
Review Comment:
I see. That is two pointers instead of one 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#issuecomment-1240681701
@pkarashchenko please review again.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on a diff in pull request #7018: wqueue: change single queue to double queue to improve speed
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on code in PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#discussion_r964548591
##########
include/nuttx/wdog.h:
##########
@@ -61,17 +61,22 @@ typedef uint32_t wdparm_t;
typedef CODE void (*wdentry_t)(wdparm_t arg);
-/* This is the internal representation of the watchdog timer structure. */
+/* This is the internal representation of the watchdog timer structure.
+ * Notice !!!
+ * Carefully with the struct wdog_s order, you may not directly modify
+ * this. This struct will combine in struct work_s in union type, and,
+ * wqueue will modify/check this struct in work_qcancel().
+ */
struct wdog_s
{
FAR struct wdog_s *next; /* Support for singly linked lists. */
+ wdparm_t arg; /* Callback argument */
Review Comment:
in struct work_s:
union
{
struct
{
struct dq_entry_s dq; /* Implements a double linked list */
clock_t qtime; /* Time work queued */
} s;
struct wdog_s timer; /* Delay expiry timer */
}
while we use WDOG_ISACTIVE(&work->timer) to decide use dq or timer,
that is error, wd_cancel() maybe modify this area, dq_rem() also can
modify this area.
So we can't use the WDOG_ISACTIVE(&work->timer) to take as the judgement,
when there is a union.
Fix:
swap the order in struct wdog_s, move the arg to the second 32bit
```
64 static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
65 FAR struct work_s *work)
66 {
67 irqstate_t flags;
68 int ret = -ENOENT;
69
70 DEBUGASSERT(work != NULL);
71
72 /* Cancelling the work is simply a matter of removing the work structure
73 * from the work queue. This must be done with interrupts disabled because
74 * new work is typically added to the work queue from interrupt handlers.
75 */
76
77 flags = enter_critical_section();
78 if (work->worker != NULL)
79 {
80 /* Remove the entry from the work queue and make sure that it is
81 * marked as available (i.e., the worker field is nullified).
82 */
83
84 if (WDOG_ISACTIVE(&work->u.timer))
85 {
86 wd_cancel(&work->u.timer);
87 }
88 else
89 {
90 dq_rem((FAR dq_entry_t *)work, &wqueue->q);
91 }
92
93 work->worker = NULL;
94 ret = OK;
95 }
96
97 leave_critical_section(flags);
98 return ret;
99 }
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org