xiaoxiang781216 commented on code in PR #10605:
URL: https://github.com/apache/nuttx/pull/10605#discussion_r1346262202
##########
sched/semaphore/CMakeLists.txt:
##########
@@ -40,4 +40,8 @@ if(CONFIG_SPINLOCK)
list(APPEND CSRCS spinlock.c)
endif()
+if(CONFIG_TICKET_SPINLOCK)
Review Comment:
where is CONFIG_TICKET_SPINLOCK defined?
##########
include/nuttx/spinlock.h:
##########
@@ -49,6 +49,21 @@ typedef uint8_t spinlock_t;
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
+#if defined(CONFIG_TICKET_SPINLOCK)
+#include <stdatomic.h>
+
+struct ticket_spinlock_s
+{
+ uint16_t owner;
+ uint16_t next;
+};
+typedef struct ticket_spinlock_s ticket_spinlock_t;
Review Comment:
```suggestion
typedef struct spinlock_s spinlock_t;
```
##########
include/nuttx/spinlock.h:
##########
@@ -49,6 +49,21 @@ typedef uint8_t spinlock_t;
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
+#if defined(CONFIG_TICKET_SPINLOCK)
Review Comment:
move before line 41:
```
# ifdef CONFIG_TICKET_SPINLOCK
struct spinlock_s
{
uint16_t owner;
uint16_t next;
};
typedef struct spinlock_s spinlock_t;
# else
/* The architecture specific spinlock.h header file must also provide the
* following:
*
* SP_LOCKED - A definition of the locked state value (usually 1)
* SP_UNLOCKED - A definition of the unlocked state value (usually 0)
* spinlock_t - The type of a spinlock memory object.
*
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
# include <arch/spinlock.h>
# endif
```
##########
sched/semaphore/spinlock.c:
##########
@@ -189,6 +224,25 @@ spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
spinlock_t spin_trylock_wo_note(FAR volatile spinlock_t *lock)
{
+#if defined(CONFIG_TICKET_SPINLOCK)
+
+ if (spin_islocked(lock))
Review Comment:
remove the check
##########
sched/semaphore/spinlock.c:
##########
@@ -139,6 +153,25 @@ void spin_lock_wo_note(FAR volatile spinlock_t *lock)
spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
{
+#if defined(CONFIG_TICKET_SPINLOCK)
Review Comment:
move aftr line 180
##########
sched/semaphore/spinlock.c:
##########
@@ -109,7 +116,14 @@ void spin_lock(FAR volatile spinlock_t *lock)
void spin_lock_wo_note(FAR volatile spinlock_t *lock)
{
+#if defined(CONFIG_TICKET_SPINLOCK)
Review Comment:
change ALL:
```suggestion
#ifdef CONFIG_TICKET_SPINLOCK
```
##########
include/nuttx/spinlock.h:
##########
@@ -49,6 +49,21 @@ typedef uint8_t spinlock_t;
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
+#if defined(CONFIG_TICKET_SPINLOCK)
+#include <stdatomic.h>
Review Comment:
remove the inclusion
##########
sched/semaphore/spinlock.c:
##########
@@ -189,6 +224,25 @@ spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
spinlock_t spin_trylock_wo_note(FAR volatile spinlock_t *lock)
Review Comment:
the return type of trylock need change to bool
##########
include/nuttx/spinlock.h:
##########
@@ -49,6 +49,21 @@ typedef uint8_t spinlock_t;
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
+#if defined(CONFIG_TICKET_SPINLOCK)
+#include <stdatomic.h>
+
+struct ticket_spinlock_s
+{
+ uint16_t owner;
+ uint16_t next;
+};
+typedef struct ticket_spinlock_s ticket_spinlock_t;
+
+#define TICKET_SPINLOCK_OWNER(l) (((struct ticket_spinlock_s *)l)->owner)
Review Comment:
remove, let's access owner and next directly
##########
include/nuttx/spinlock.h:
##########
@@ -49,6 +49,21 @@ typedef uint8_t spinlock_t;
* SP_LOCKED and SP_UNLOCKED must be constants of type spinlock_t.
*/
+#if defined(CONFIG_TICKET_SPINLOCK)
+#include <stdatomic.h>
+
+struct ticket_spinlock_s
Review Comment:
```suggestion
union spinlock_u
{
struct
{
uint16_t owner;
uint16_t next;
};
uint32_t value;
};
typedef struct spinlock_u spinlock_t;
```
##########
include/nuttx/spinlock.h:
##########
@@ -285,7 +300,11 @@ void spin_unlock_wo_note(FAR volatile spinlock_t *lock);
****************************************************************************/
/* bool spin_islocked(FAR spinlock_t lock); */
+#if defined(CONFIG_TICKET_SPINLOCK)
+#define spin_islocked(l) (TICKET_SPINLOCK_OWNER(l) != TICKET_SPINLOCK_NEXT(l))
Review Comment:
```suggestion
# define spin_islocked(l) (TICKET_SPINLOCK_OWNER(l) !=
TICKET_SPINLOCK_NEXT(l))
```
##########
include/nuttx/spinlock.h:
##########
@@ -285,7 +300,11 @@ void spin_unlock_wo_note(FAR volatile spinlock_t *lock);
****************************************************************************/
/* bool spin_islocked(FAR spinlock_t lock); */
+#if defined(CONFIG_TICKET_SPINLOCK)
+#define spin_islocked(l) (TICKET_SPINLOCK_OWNER(l) != TICKET_SPINLOCK_NEXT(l))
+#else
#define spin_islocked(l) (*(l) == SP_LOCKED)
Review Comment:
```suggestion
# define spin_islocked(l) (*(l) == SP_LOCKED)
```
##########
sched/semaphore/spinlock.c:
##########
@@ -109,7 +116,14 @@ void spin_lock(FAR volatile spinlock_t *lock)
void spin_lock_wo_note(FAR volatile spinlock_t *lock)
{
+#if defined(CONFIG_TICKET_SPINLOCK)
+
+ atomic_uint ticket = atomic_fetch_add(&TICKET_SPINLOCK_NEXT(lock), 1);
Review Comment:
```suggestion
uint16_t ticket = atomic_fetch_add(&lock->next, 1);
```
##########
sched/semaphore/spinlock.c:
##########
@@ -189,6 +224,25 @@ spinlock_t spin_trylock(FAR volatile spinlock_t *lock)
spinlock_t spin_trylock_wo_note(FAR volatile spinlock_t *lock)
{
+#if defined(CONFIG_TICKET_SPINLOCK)
+
+ if (spin_islocked(lock))
+ {
+ return SP_LOCKED;
+ }
+
+ ticket_spinlock_t old = atomic_load((ticket_spinlock_t *)lock);
Review Comment:
```
#ifdef CONFIG_TICKET_SPINLOCK
uint16_t ticket = atomic_load(&lock->next);
spinlock_t old =
{
ticket, ticket
};
spinlock_t new =
{
ticket, ticket + 1
};
if (atomic_compare_exchange_strong(lock.value, &old.value, new.value))
#else
if (up_testset(lock) == SP_LOCKED)
#endif
```
--
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
