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/01/13 08:55:35 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #5216: [SMP]sched: task list should be protected by global spinlock
anchao opened a new pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216
## Summary
sched: task list should be protected by global spinlock
Revert "sched: sched: Remove sched_tasklistlock.c for SMP"
armv7-a dataabort:
```
-----------------------------------------------------------------
| up_assert: Assertion failed CPU0 at file:armv7-a/arm_dataabort.c line: 160 task: event_dispatch
| up_dumpstate: Call Trace:
| backtrace|17: 0x3c0221d8 0x3c03c2d4 0x3c028f6c 0x3c018634 0x3c017d04 0x3c04b5c4 0x3c053504 0x3c017530
| up_registerdump: R0: 3c5829f8 3c2bcd08 00000000 3c5e2730 3c2bcd08 3c5e2730 00000000 3c2bc700
| up_registerdump: R8: 3c2bcd5a 3c2bc2f4 00000000 3c582a14 00000000 3c5829f8 3c01c17c 3c0221d8
| up_registerdump: CPSR: 200000d3
| up_dumpstate: Current sp: 3c582848
| up_dumpstate: Interrupt stack:
| up_dumpstate: base: 3c2bb0e8
| up_dumpstate: size: 00001000
| up_dumpstate: used: 00000208
| up_dumpstate: User stack:
| up_dumpstate: base: 3c582bd8
| up_dumpstate: size: 00002804
| up_dumpstate: used: 00001180
| up_dumpstate: ERROR: Stack pointer is not within the interrupt stack
| up_dumpstate: User sp: 3c5829f8
| up_dumpstate: User Stack
| up_dumpstate: CPU0:
| up_showtasks: PID PRI USED STACK FILLED CPU COMMAND
| up_showtasks: ---- ---- 520 4096 12.6% ---- irq
| up_dump_task: 0 0 736 4076 18.0% 0.0% CPU0 IDLE
| up_dump_task: 1 0 344 4076 8.4% 0.0% CPU1 IDLE
| up_dump_task: 3 253 936 4068 23.0% 0.0% hpwork
| up_dump_task: 4 253 936 4068 23.0% 0.0% hpwork
| up_dump_task: 5 110 504 4068 12.3% 0.0% lpwork
| up_dump_task: 6 110 504 4068 12.3% 0.0% lpwork
| ...
| up_dump_task: 17 225 4480 10244 43.7% 0.0% event_dispatch
-----------------------------------------------------------------
```
Task list corruption:
```
-----------------------------------------------------------------
| nuttx$ arm-none-eabi-addr2line -e nuttx 0x3c0221d8 0x3c03c2d4 0x3c028f6c 0x3c018634 0x3c017d04 0x3c04b5c4 0x3c053504 0x3c017530
| nuttx/libs/libc/queue/dq_cat.c:72
| nuttx/sched/sched/sched_mergepending.c:270
| nuttx/arch/arm/src/armv7-a/arm_releasepending.c:58
| nuttx/sched/sched/sched_unlock.c:141
| nuttx/sched/pthread/pthread_condwait.c:129
| external/pns/src/event_dispatch.c:1113
| external/pns/src/common_worker.c:26 (discriminator 3)
| nuttx/sched/pthread/pthread_create.c:191
-----------------------------------------------------------------
```
This reverts commit 1dad55d4bef2cd078ee51b949abc86f6b58f2e7d.
Signed-off-by: chao.an <an...@xiaomi.com>
## Impact
Task list en/dequeue on SMP
## Testing
Products online test which already on the market
--
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] anchao commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012834331
> > please check the attachment
> > config.txt
>
> @anchao Thanks for the file. Let me clarify which codebase you are using for your product. As I commented, SMP related bugs reported in slide page 8 are very important. So please confirm all of the PRs have been merged to your codebase.
@masayuki2009 san,
Thanks a lot for the information! The code base has been synchronized with mainline, since this is just a workaround solution, let me close this issue, any update I will create a new pull request.
--
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] masayuki2009 commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012118983
>This issue only reproduced on the product device
@anchao
Can you attach the .config file (not defconfig) with which you had the issue?
--
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] anchao commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012110474
> @masayuki2009 san,
>
> > I think the root cause of the issue would be different. As I commented, we need to check sched_lock() related code when
> > the issue happens. So the code should be selectable by Kconfig (default = n) to show that it's just a workaround.
Yes, I am investigating if there are other side effects that could be causing this issue.
> > I understand your points but please remember that sched_tasklistlock.c was just a workaround to avoid memory corruption.
do you remember witch PR resolved the issue you had before?
--
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] masayuki2009 commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012070962
>This issue only reproduced on the product device, I have prepared some test case but which difficult to reproduce this >situation. after extensive testing, I confirm this issue will disappear if bring back your commit.
@anchao
I understand your points but please remember that sched_tasklistlock.c was just a workaround to avoid memory corruption.
I think the root cause of the issue would be different. As I commented, we need to check sched_lock() related code when the issue happens. So the code should be selectable by Kconfig (default = n) to show that it's just a workaround.
@xiaoxiang781216
Do you have any comments?
--
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] masayuki2009 commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012116463
>Do you remember witch PR resolved the issue you had before?
@anchao
Please refer to my slide (page 8) at NuttX 2021.
current_status_of_the_nuttx_smp_kernel_ishikawa_20210816
--
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] anchao commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012110228
@masayuki2009 san,
> I think the root cause of the issue would be different. As I commented, we need to check sched_lock() related code when
> the issue happens. So the code should be selectable by Kconfig (default = n) to show that it's just a workaround.
Yes, I am investigating if there are other side effects that could be causing this issue.
> I understand your points but please remember that sched_tasklistlock.c was just a workaround to avoid memory corruption.
do you remember witch PR resolved the issue you had before?
--
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] anchao commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1011948659
> Do you have any test cases so that we can reproduce the issue? In my understanding, we do not need the spinlock for the tasklist. There still might be another issue such as sched_lock().
@masayuki2009 san,
This issue only reproduced on the product device, I have prepared some test case but which difficult to reproduce this situation. after extensive testing, I confirm this issue will disappear if bring back your commit.
--
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] anchao commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012220808
> > Can you attach the .config file (not defconfig) with which you had the issue?
@masayuki2009 san,
please check the attachment
[config.txt](https://github.com/apache/incubator-nuttx/files/7863730/config.txt)
--
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] masayuki2009 edited a comment on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1011936035
@anchao
Do you have any test cases so that we can reproduce the issue?
In my understanding, we do not need the spinlock for the tasklist.
There still might be another issue such as sched_lock().
See https://github.com/apache/incubator-nuttx/issues/3600
--
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] masayuki2009 edited a comment on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012630690
>please check the attachment
>config.txt
@anchao
Thanks for the file.
Let me clarify which codebase you are using for your product.
As I commented, SMP related bugs reported in slide page 8 are very important.
So please confirm all of the PRs have been merged to your codebase.
--
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] anchao closed pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
anchao closed pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216
--
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] masayuki2009 commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1011936035
@anchao
Do you have any test cases so that we can reproduce the issue?
In my understanding, we do not need the spinlock for the tasklist.
There still might be another issue such as sched_lock().
--
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] masayuki2009 commented on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012630690
>please check the attachment
>config.txt
@anchao
Thanks for the file.
Let me clarify which code base are you using for your product.
As I commented, SMP related bugs reported in slide page 8 are very important.
So please confirm all of the PR have been merged to your code base.
--
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] masayuki2009 edited a comment on pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#issuecomment-1012116463
>Do you remember witch PR resolved the issue you had before?
@anchao
Please refer to my slide (page 8) at NuttX 2021.
[current_status_of_the_nuttx_smp_kernel_ishikawa_20210816.pdf](https://github.com/apache/incubator-nuttx/files/7862921/current_status_of_the_nuttx_smp_kernel_ishikawa_20210816.pdf)
--
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 change in pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#discussion_r783973155
##########
File path: sched/sched/sched_tasklistlock.c
##########
@@ -0,0 +1,118 @@
+/****************************************************************************
+ * sched/sched/sched_tasklistlock.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 <nuttx/config.h>
+#include <nuttx/spinlock.h>
+
+#include <sys/types.h>
+#include <arch/irq.h>
+
+#include "sched/sched.h"
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/* Splinlock to protect the tasklists */
+
+static volatile spinlock_t g_tasklist_lock = SP_UNLOCKED;
+
+/* Handles nested calls */
+
+static volatile uint8_t g_tasklist_lock_count[CONFIG_SMP_NCPUS];
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nxsched_lock_tasklist()
+ *
+ * Description:
+ * Disable local interrupts and take the global spinlock (g_tasklist_lock)
+ * if the call counter (g_tasklist_lock_count[cpu]) equals to 0. Then the
+ * counter on the CPU is incremented to allow nested call.
+ *
+ * NOTE: This API is used to protect tasklists in the scheduler. So do not
+ * use this API for other purposes.
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to nxsched_lock_tasklist();
+ ****************************************************************************/
+
+irqstate_t nxsched_lock_tasklist(void)
+{
+ int me;
+ irqstate_t ret;
Review comment:
```suggestion
irqstate_t flags;
```
--
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 change in pull request #5216: [SMP]sched: task list should be protected by global spinlock
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5216:
URL: https://github.com/apache/incubator-nuttx/pull/5216#discussion_r783972189
##########
File path: sched/sched/sched_tasklistlock.c
##########
@@ -0,0 +1,118 @@
+/****************************************************************************
+ * sched/sched/sched_tasklistlock.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 <nuttx/config.h>
+#include <nuttx/spinlock.h>
+
Review comment:
```suggestion
#include <assert.h>
```
--
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