xiaoxiang781216 commented on code in PR #10605:
URL: https://github.com/apache/nuttx/pull/10605#discussion_r1347658375
##########
include/nuttx/spinlock.h:
##########
@@ -352,9 +383,11 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int
cpu,
****************************************************************************/
/* void spin_initialize(FAR spinlock_t *lock, spinlock_t state); */
-
-#define spin_initialize(l,s) do { *(l) = (s); } while (0)
-
+#ifdef CONFIG_TICKET_SPINLOCK
+# define spin_initialize(l,s) do { (*l).value = (s); } while (0)
Review Comment:
don't need change.
##########
include/nuttx/spinlock.h:
##########
@@ -48,9 +68,12 @@ typedef uint8_t spinlock_t;
*
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
-
Review Comment:
revert
##########
include/nuttx/spinlock.h:
##########
@@ -84,6 +107,10 @@ typedef uint8_t spinlock_t;
# define __SP_UNLOCK_FUNCTION 1
#endif
+#ifdef CONFIG_TICKET_SPINLOCK
+# define __SP_UNLOCK_FUNCTION 1
Review Comment:
move after line 47
##########
sched/semaphore/spinlock.c:
##########
@@ -71,7 +75,14 @@ void spin_lock(FAR volatile spinlock_t *lock)
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
+#ifdef CONFIG_TICKET_SPINLOCK
+
+ uint16_t ticket = atomic_fetch_add(&lock->tickets.next, 1);
+ while (atomic_load(&lock->tickets.owner) != ticket)
+
+#else // CONFIG_SPINLOCK
Review Comment:
```suggestion
#else /* CONFIG_TICKET_SPINLOCK */
##########
sched/semaphore/spinlock.c:
##########
@@ -137,32 +155,52 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
+bool spin_trylock(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we are waiting for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
- if (up_testset(lock) == SP_LOCKED)
+#ifdef CONFIG_TICKET_SPINLOCK
+ uint16_t ticket = atomic_load(&lock->tickets.next);
+
+ spinlock_t old =
+ {
+ {
+ ticket, ticket
+ }
+ };
+
+ spinlock_t new =
+ {
+ {
+ ticket, ticket + 1
+ }
+ };
+
+ if (atomic_compare_exchange_strong(&lock->value, &old.value, new.value))
+#else /* CONFIG_TICKET_SPINLOCK */
+ if (up_testset(lock) == SP_UNLOCKED)
+#endif /* CONFIG_TICKET_SPINLOCK */
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we abort for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_ABORT);
#endif
- SP_DSB();
- return SP_LOCKED;
+ SP_DMB();
+ return SP_UNLOCKED;
Review Comment:
return false
##########
sched/semaphore/spinlock.c:
##########
@@ -137,32 +155,52 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
+bool spin_trylock(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we are waiting for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
- if (up_testset(lock) == SP_LOCKED)
+#ifdef CONFIG_TICKET_SPINLOCK
+ uint16_t ticket = atomic_load(&lock->tickets.next);
+
+ spinlock_t old =
+ {
+ {
+ ticket, ticket
+ }
+ };
+
+ spinlock_t new =
+ {
+ {
+ ticket, ticket + 1
+ }
+ };
+
+ if (atomic_compare_exchange_strong(&lock->value, &old.value, new.value))
+#else /* CONFIG_TICKET_SPINLOCK */
+ if (up_testset(lock) == SP_UNLOCKED)
+#endif /* CONFIG_TICKET_SPINLOCK */
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we abort for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_ABORT);
#endif
- SP_DSB();
- return SP_LOCKED;
+ SP_DMB();
+ return SP_UNLOCKED;
}
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we have the spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCKED);
#endif
- SP_DMB();
- return SP_UNLOCKED;
+ SP_DSB();
+ return SP_LOCKED;
Review Comment:
return true
##########
sched/semaphore/spinlock.c:
##########
@@ -366,7 +436,11 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int
cpu,
prev = *set;
#endif
*set &= ~(1 << cpu);
+#ifdef CONFIG_TICKET_SPINLOCK
+ orlock->value = (*set != 0) ? SP_LOCKED : SP_UNLOCKED;
Review Comment:
don't need with the new definition of SP_LOCKED and SP_UNLOCKED
##########
sched/irq/irq_csection.c:
##########
@@ -46,7 +46,7 @@
* disabled.
*/
-volatile spinlock_t g_cpu_irqlock = SP_UNLOCKED;
+volatile spinlock_t g_cpu_irqlock = INIT_SPINLOCK_VALUE;
Review Comment:
revert the change
##########
include/nuttx/spinlock.h:
##########
@@ -48,9 +68,12 @@ typedef uint8_t spinlock_t;
*
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
-
#include <arch/spinlock.h>
+#define INIT_SPINLOCK_VALUE SP_UNLOCKED
Review Comment:
SPINLOCK_INITIALIZER
##########
sched/semaphore/spinlock.c:
##########
@@ -71,7 +75,14 @@ void spin_lock(FAR volatile spinlock_t *lock)
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
+#ifdef CONFIG_TICKET_SPINLOCK
+
+ uint16_t ticket = atomic_fetch_add(&lock->tickets.next, 1);
+ while (atomic_load(&lock->tickets.owner) != ticket)
+
Review Comment:
ditto
##########
include/nuttx/spinlock.h:
##########
@@ -39,6 +39,26 @@
typedef uint8_t spinlock_t;
#else
+#ifdef CONFIG_TICKET_SPINLOCK
+
+# define SP_UNLOCKED 0 /* The Un-locked state */
+# define SP_LOCKED 1 /* The Locked state */
+
+# define INIT_SPINLOCK_VALUE {{SP_UNLOCKED}}
+
+union spinlock_u
+{
+ struct
+ {
+ uint16_t owner;
+ uint16_t next;
+ } tickets;
Review Comment:
ticket
##########
sched/semaphore/spinlock.c:
##########
@@ -109,7 +120,14 @@ void spin_lock(FAR volatile spinlock_t *lock)
void spin_lock_wo_note(FAR volatile spinlock_t *lock)
{
+#ifdef CONFIG_TICKET_SPINLOCK
+
Review Comment:
ditto
##########
sched/semaphore/spinlock.c:
##########
@@ -71,7 +75,14 @@ void spin_lock(FAR volatile spinlock_t *lock)
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
+#ifdef CONFIG_TICKET_SPINLOCK
+
Review Comment:
remove the blank line
##########
include/nuttx/spinlock.h:
##########
@@ -39,6 +39,26 @@
typedef uint8_t spinlock_t;
#else
+#ifdef CONFIG_TICKET_SPINLOCK
+
+# define SP_UNLOCKED 0 /* The Un-locked state */
+# define SP_LOCKED 1 /* The Locked state */
+
+# define INIT_SPINLOCK_VALUE {{SP_UNLOCKED}}
Review Comment:
remove, don't need
##########
sched/semaphore/spinlock.c:
##########
@@ -109,7 +120,14 @@ void spin_lock(FAR volatile spinlock_t *lock)
void spin_lock_wo_note(FAR volatile spinlock_t *lock)
{
+#ifdef CONFIG_TICKET_SPINLOCK
+
+ uint16_t ticket = atomic_fetch_add(&lock->tickets.next, 1);
+ while (atomic_load(&lock->tickets.owner) != ticket)
+
Review Comment:
ditto
##########
sched/semaphore/spinlock.c:
##########
@@ -137,32 +155,52 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
+bool spin_trylock(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we are waiting for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
- if (up_testset(lock) == SP_LOCKED)
+#ifdef CONFIG_TICKET_SPINLOCK
+ uint16_t ticket = atomic_load(&lock->tickets.next);
+
+ spinlock_t old =
+ {
+ {
+ ticket, ticket
+ }
+ };
+
+ spinlock_t new =
+ {
+ {
+ ticket, ticket + 1
+ }
+ };
+
+ if (atomic_compare_exchange_strong(&lock->value, &old.value, new.value))
+#else /* CONFIG_TICKET_SPINLOCK */
+ if (up_testset(lock) == SP_UNLOCKED)
+#endif /* CONFIG_TICKET_SPINLOCK */
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we abort for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_ABORT);
#endif
- SP_DSB();
- return SP_LOCKED;
+ SP_DMB();
Review Comment:
why change
##########
sched/semaphore/spinlock.c:
##########
@@ -137,32 +155,52 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
+bool spin_trylock(FAR volatile spinlock_t *lock)
Review Comment:
correct the comment at line 150-151
##########
sched/semaphore/spinlock.c:
##########
@@ -137,32 +155,52 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
+bool spin_trylock(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we are waiting for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
- if (up_testset(lock) == SP_LOCKED)
+#ifdef CONFIG_TICKET_SPINLOCK
+ uint16_t ticket = atomic_load(&lock->tickets.next);
+
+ spinlock_t old =
+ {
+ {
+ ticket, ticket
+ }
+ };
+
+ spinlock_t new =
+ {
+ {
Review Comment:
ditto
##########
include/nuttx/spinlock.h:
##########
@@ -39,6 +39,26 @@
typedef uint8_t spinlock_t;
#else
+#ifdef CONFIG_TICKET_SPINLOCK
+
+# define SP_UNLOCKED 0 /* The Un-locked state */
Review Comment:
```
# define SP_UNLOCKED {union spinlock_u}{{0, 0}}
# define SP_LOCKED {union spinlock_u}{{0, 1}}
```
##########
sched/semaphore/spinlock.c:
##########
@@ -137,32 +155,52 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
+bool spin_trylock(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we are waiting for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
- if (up_testset(lock) == SP_LOCKED)
+#ifdef CONFIG_TICKET_SPINLOCK
+ uint16_t ticket = atomic_load(&lock->tickets.next);
+
+ spinlock_t old =
+ {
+ {
Review Comment:
add indent
##########
sched/semaphore/spinlock.c:
##########
@@ -137,32 +155,52 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
+bool spin_trylock(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we are waiting for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCK);
#endif
- if (up_testset(lock) == SP_LOCKED)
+#ifdef CONFIG_TICKET_SPINLOCK
+ uint16_t ticket = atomic_load(&lock->tickets.next);
+
+ spinlock_t old =
+ {
+ {
+ ticket, ticket
+ }
+ };
+
+ spinlock_t new =
+ {
+ {
+ ticket, ticket + 1
+ }
+ };
+
+ if (atomic_compare_exchange_strong(&lock->value, &old.value, new.value))
+#else /* CONFIG_TICKET_SPINLOCK */
+ if (up_testset(lock) == SP_UNLOCKED)
+#endif /* CONFIG_TICKET_SPINLOCK */
{
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we abort for a spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_ABORT);
#endif
- SP_DSB();
- return SP_LOCKED;
+ SP_DMB();
+ return SP_UNLOCKED;
}
#ifdef CONFIG_SCHED_INSTRUMENTATION_SPINLOCKS
/* Notify that we have the spinlock */
sched_note_spinlock(this_task(), lock, NOTE_SPINLOCK_LOCKED);
#endif
- SP_DMB();
- return SP_UNLOCKED;
+ SP_DSB();
Review Comment:
why change to SP_DSB
##########
sched/semaphore/spinlock.c:
##########
@@ -303,7 +369,11 @@ void spin_setbit(FAR volatile cpu_set_t *set, unsigned int
cpu,
prev = *set;
#endif
*set |= (1 << cpu);
+#ifdef CONFIG_TICKET_SPINLOCK
+ orlock->value = SP_LOCKED;
Review Comment:
revert the change
##########
sched/semaphore/spinlock.c:
##########
@@ -187,16 +225,36 @@ spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
*
****************************************************************************/
-spinlock_t spin_trylock_wo_note(FAR volatile spinlock_t *lock)
+bool spin_trylock_wo_note(FAR volatile spinlock_t *lock)
{
- if (up_testset(lock) == SP_LOCKED)
+#ifdef CONFIG_TICKET_SPINLOCK
+ uint16_t ticket = atomic_load(&lock->tickets.next);
+
+ spinlock_t old =
{
- SP_DSB();
- return SP_LOCKED;
+ {
Review Comment:
let' add more indent
--
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
