You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "hujun260 (via GitHub)" <gi...@apache.org> on 2024/05/20 02:32:23 UTC
[PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
hujun260 opened a new pull request, #12376:
URL: https://github.com/apache/nuttx/pull/12376
## Summary
we can use g_cpu_lockset to determine whether we are currently in the scheduling lock, and all accesses and modifications to g_cpu_lockset, g_cpu_irqlock, g_cpu_irqset are in the critical section, so we can directly operate on it.
## Impact
## Testing
test:
We can use qemu for testing.
compiling
make distclean -j20; ./tools/configure.sh -l qemu-armv8a:nsh_smp ;make -j20 running
qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic -machine virt,virtualization=on,gic-version=3 -net none -chardev stdio,id=con,mux=on -serial chardev:con -mon chardev=con,mode=readline -kernel ./nuttx
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642328072
##########
include/nuttx/irq.h:
##########
@@ -70,6 +70,18 @@
#endif /* __ASSEMBLY__ */
+#ifdef CONFIG_SMP
+# define cpu_irqlock_clear() \
+ do \
+ { \
+ g_cpu_irqset = 0; \
+ SP_DMB(); \
+ g_cpu_irqlock = SP_UNLOCKED; \
+ SP_DSB(); \
+ } \
+ while (0)
Review Comment:
Got it, thank for your explaination. This idea is very great. Thanks.
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642059121
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
Thanks for your explaination. I notice the caller of all `spin_setbit` need to call `enter_critical_section` 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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1639718268
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
> Look like scheduler lock can be token by two cpus?
yes, The current implementation of sched_lock is that whenever any CPU acquires the sched_lock, it causes the sched_lock to be in effect for all CPUs. There will be subsequent patchs to address this 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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1618009915
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
this_task() is a function call that involves disabling interrupts and this_cpu, since restore_critical_section is always called in an interrupt-disabled context, there's no need to disable interrupts again.
Therefore, to save time and achieve the same effect, I directly call tcb = current_task(me) instead of tcb = this_task().
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
this_task() is a function call that involves disabling interrupts and this_cpu(), since restore_critical_section is always called in an interrupt-disabled context, there's no need to disable interrupts again.
Therefore, to save time and achieve the same effect, I directly call tcb = current_task(me) instead of tcb = this_task().
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1634813499
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
@hujun260
Thanks for dividing the commit.
However, I think the new commit message is still unclear and has long lines.
So, could you change the 2nd commit message as follows?
```
Author: hujun5 <hu...@xiaomi.com>
Date: Tue Jun 11 08:05:26 2024 +0800
sched: irq: Change this_task to current_task(me) in restore_critical_section()
this_task() is a function call that involves disabling interrupts and this_cpu().
Since restore_critical_section is always called in an interrupt-disabled context,
there's no need to disable interrupts again. Therefore, to save time and achieve
the same effect, I directly call tcb = current_task(me) instead of tcb = this_task().
Signed-off-by: hujun5 <hu...@xiaomi.com>
```
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642279543
##########
include/nuttx/irq.h:
##########
@@ -70,6 +70,18 @@
#endif /* __ASSEMBLY__ */
+#ifdef CONFIG_SMP
+# define cpu_irqlock_clear() \
+ do \
+ { \
+ g_cpu_irqset = 0; \
+ SP_DMB(); \
+ g_cpu_irqlock = SP_UNLOCKED; \
+ SP_DSB(); \
+ } \
+ while (0)
Review Comment:
"g_cpu_irqset = 0" can save at least one CPU instruction compared to "g_cpu_irqset &= ~(1 << cpu);"
If adding "DEBUGASSERT(g_cpu_irqset == 0);" in "cpu_irqlock_set(cpu)" can also achieve the effect you want?
for example:
# define cpu_irqlock_set(cpu) \
do \
{ \
DEBUGASSERT(g_cpu_irqset == 0); \
g_cpu_irqset |= (1 << cpu); \
} \
while (0)
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642328072
##########
include/nuttx/irq.h:
##########
@@ -70,6 +70,18 @@
#endif /* __ASSEMBLY__ */
+#ifdef CONFIG_SMP
+# define cpu_irqlock_clear() \
+ do \
+ { \
+ g_cpu_irqset = 0; \
+ SP_DMB(); \
+ g_cpu_irqlock = SP_UNLOCKED; \
+ SP_DSB(); \
+ } \
+ while (0)
Review Comment:
Got it, thank for your explaination. This is very great and better than my idea. Thanks.
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1639207147
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
Look like scheduler lock can be token by two cpus?
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1615197919
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
@hujun260
Please tell us why did you change this line in this PR.
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1619057021
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
@masayuki2009 do you have more concern?
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642279543
##########
include/nuttx/irq.h:
##########
@@ -70,6 +70,18 @@
#endif /* __ASSEMBLY__ */
+#ifdef CONFIG_SMP
+# define cpu_irqlock_clear() \
+ do \
+ { \
+ g_cpu_irqset = 0; \
+ SP_DMB(); \
+ g_cpu_irqlock = SP_UNLOCKED; \
+ SP_DSB(); \
+ } \
+ while (0)
Review Comment:
"g_cpu_irqset = 0" can save at least one CPU instruction compared to "g_cpu_irqset &= ~(1 << cpu);"
If adding "DEBUGASSERT(g_cpu_irqset == 0);" in "cpu_irqlock_set(cpu)" can also achieve the effect you want?
for example:
```
# define cpu_irqlock_set(cpu) \
do \
{ \
DEBUGASSERT(g_cpu_irqset == 0); \
g_cpu_irqset |= (1 << cpu); \
} \
while (0)
```
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1630749486
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
@hujun260
Please add a separate commit (not a PR) for it.
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642059121
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
Thanks for your explaination. I notice the caller of all `spin_setbit` need to be call in `enter_critical_section` 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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642059121
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
Thanks for your explaination. I notice the caller of all `spin_setbit` is called in `enter_critical_section` 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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1619057021
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
@masayuki2009 do you have more concern
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1640882350
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
Not related performance issue.
If there is any resource protected by `g_cpu_schedlock` but we can get scheduler lock with different cpus simultaneously we will face race issue.
If I am wrong, please correct me.
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642016276
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
This is a common misunderstanding about sched_lock. It cannot achieve mutual exclusion of resources, especially in SMP scenarios.
In most cases, if we want protect resources, enter_critical_section should be used.
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642082111
##########
include/nuttx/irq.h:
##########
@@ -70,6 +70,18 @@
#endif /* __ASSEMBLY__ */
+#ifdef CONFIG_SMP
+# define cpu_irqlock_clear() \
+ do \
+ { \
+ g_cpu_irqset = 0; \
+ SP_DMB(); \
+ g_cpu_irqlock = SP_UNLOCKED; \
+ SP_DSB(); \
+ } \
+ while (0)
Review Comment:
nit: maybe it can be
```
# define cpu_irqlock_clear(cpu) \
do \
{ \
g_cpu_irqset &= ~(1 << cpu); \
SP_DMB(); \
g_cpu_irqlock = SP_UNLOCKED; \
SP_DSB(); \
} \
while (0)
```
similar with `cpu_irqlock_set(cpu)`
There is a benefit, it can be used to trace is there different cpus enter critical section simultaneously.
WDYT?
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#issuecomment-2127047566
@masayuki2009 I will merge it since you didn't add any 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1640882350
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
Not related performance issue.
If there is any resource protected by `g_cpu_schedlock` but we can get scheduler lock with different cpus simultaneously we will face race issue.
If I am wrong, please correct me.
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1618009915
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
this_task() is a function call that involves disabling interrupts, and since restore_critical_section is always called in an interrupt-disabled context, there's no need to disable interrupts again.
Therefore, to save time and achieve the same effect, I directly call tcb = current_task(me) instead of tcb = this_task().
--
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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1642059121
##########
sched/sched/sched_lock.c:
##########
@@ -171,18 +131,17 @@ int sched_lock(void)
* locked (or we would not be executing!).
*/
- spin_setbit(&g_cpu_lockset, this_cpu(), &g_cpu_locksetlock,
- &g_cpu_schedlock);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) == 0);
+ g_cpu_lockset |= (1 << cpu);
}
else
{
/* If this thread already has the scheduler locked, then
- * g_cpu_schedlock() should indicate that the scheduler is locked
+ * g_cpu_lockset should indicate that the scheduler is locked
* and g_cpu_lockset should include the bit setting for this CPU.
*/
- DEBUGASSERT(spin_is_locked(&g_cpu_schedlock) &&
- (g_cpu_lockset & (1 << this_cpu())) != 0);
+ DEBUGASSERT((g_cpu_lockset & (1 << cpu)) != 0);
}
Review Comment:
Thanks for your explaination. I notice the caller of all `spin_setbit` need to be called in `enter_critical_section` 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
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1634930528
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sched：remove g_cpu_schedlock g_cpu_irqsetlock g_cpu_locksetlock [nuttx]
Posted by "hujun260 (via GitHub)" <gi...@apache.org>.
hujun260 commented on code in PR #12376:
URL: https://github.com/apache/nuttx/pull/12376#discussion_r1634201601
##########
sched/irq/irq_csection.c:
##########
@@ -615,13 +613,14 @@ void restore_critical_section(void)
* followed by context switching.
*/
- FAR struct tcb_s *tcb = this_task();
+ FAR struct tcb_s *tcb;
int me = this_cpu();
/* Adjust global IRQ controls. If irqcount is greater than zero,
* then this task/this CPU holds the IRQ lock
*/
+ tcb = current_task(me);
Review Comment:
> @hujun260 Please add a separate commit (not a PR) for it.
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org