xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1348495710
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +389,149 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned
int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this CPU does not already hold the spinlock, then loop until the
+ * spinlock is successfully locked.
+ *
+ * This implementation is non-reentrant and set a bit of lock.
+ *
+ * The priority of reader is higher than writter if a reader hold the
+ * lock, a new reader can get its lock but writer can't get this lock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to lock.
+ *
+ * Returned Value:
+ * None. When the function returns, the spinlock was successfully locked
+ * by this CPU.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void read_lock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old;
+
+ while (true)
+ {
+ old = atomic_load(lock);
+
+ if (old == SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &zero, 1))
+ {
+ break;
+ }
+ else if(old > SP_UNLOCKED)
+ {
+ atomic_fetch_add(lock, 1);
+ break;
+ }
+
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_unlock
+ *
+ * Description:
+ * Release a bit on a non-reentrant spinlock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to unlock.
+ *
+ * Returned Value:
+ * None.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void read_unlock(FAR volatile rwlock_t *lock)
+{
+ DEBUGASSERT(atomic_load(lock) > 0);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this CPU does not already hold the spinlock, then loop until the
+ * spinlock is successfully locked.
+ *
+ * This implementation is non-reentrant and set all bit on lock to avoid
+ * readers and writers.
+ *
+ * The priority of reader is higher than writter if a reader hold the
+ * lock, a new reader can get its lock but writer can't get this lock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to lock.
+ *
+ * Returned Value:
+ * None. When the function returns, the spinlock was successfully locked
+ * by this CPU.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void write_lock(FAR volatile rwlock_t *lock)
+{
+ while (!atomic_compare_exchange_strong(lock, &zero, -1))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_unlock
+ *
+ * Description:
+ * Release all bit on a non-reentrant spinlock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to unlock.
+ *
+ * Returned Value:
+ * None.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void write_unlock(FAR volatile rwlock_t *lock)
+{
+ /* Ensure this cpu already get write lock */
+
+ DEBUGASSERT(atomic_load(lock) == -1);
+
+ SP_DMB();
+ atomic_store(lock, SP_UNLOCKED);
Review Comment:
don't use SP_UNLOCKED/SP_LOCKED which is incompatible with ticket spinlock.
##########
sched/semaphore/spinlock.c:
##########
@@ -32,6 +32,11 @@
#include <nuttx/sched_note.h>
#include <arch/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+# include <stdatomic.h>
+const static rwlock_t zero = SP_UNLOCKED;
Review Comment:
let's change to local variable to save the memory
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +389,149 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned
int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this CPU does not already hold the spinlock, then loop until the
+ * spinlock is successfully locked.
+ *
+ * This implementation is non-reentrant and set a bit of lock.
+ *
+ * The priority of reader is higher than writter if a reader hold the
+ * lock, a new reader can get its lock but writer can't get this lock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to lock.
+ *
+ * Returned Value:
+ * None. When the function returns, the spinlock was successfully locked
+ * by this CPU.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void read_lock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old;
+
+ while (true)
Review Comment:
```
while (true)
{
rwlock_t old = atomic_load(lock);
if (old >= 0 &&
atomic_compare_exchange_strong(lock, &old, old + 1))
{
break;
}
else if (old < 0)
{
SP_DSB();
SP_WFE();
}
}
```
##########
include/nuttx/spinlock.h:
##########
@@ -51,6 +51,11 @@ typedef uint8_t spinlock_t;
#include <arch/spinlock.h>
+#if defined(CONFIG_RW_SPINLOCK)
Review Comment:
change to:
```
#ifdef CONFIG_RW_SPINLOCK
```
##########
include/nuttx/spinlock.h:
##########
@@ -51,6 +51,10 @@ typedef uint8_t spinlock_t;
#include <arch/spinlock.h>
+#if defined(CONFIG_RW_SPINLOCK)
+typedef uint32_t rwlock_t;
Review Comment:
```suggestion
typedef int32_t rwlock_t;
```
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +389,149 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned
int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this CPU does not already hold the spinlock, then loop until the
+ * spinlock is successfully locked.
+ *
+ * This implementation is non-reentrant and set a bit of lock.
+ *
+ * The priority of reader is higher than writter if a reader hold the
+ * lock, a new reader can get its lock but writer can't get this lock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to lock.
+ *
+ * Returned Value:
+ * None. When the function returns, the spinlock was successfully locked
+ * by this CPU.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void read_lock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old;
+
+ while (true)
+ {
+ old = atomic_load(lock);
+
+ if (old == SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &zero, 1))
+ {
+ break;
+ }
+ else if(old > SP_UNLOCKED)
+ {
+ atomic_fetch_add(lock, 1);
Review Comment:
still has the race condition:
1. thread 1: atomic_load-> count return 1 at line 424
2. thread 2: read_unlock-> count change to 0
3. thread 2: write_lock -> coont change to -1
4. thread 1: atomic_fetch_add -> count change to 0
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +453,98 @@ void spin_unlock_irqrestore_wo_note(FAR spinlock_t *lock,
irqstate_t flags);
# define spin_unlock_irqrestore_wo_note(l, f) up_irq_restore(f)
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this CPU does not already hold the spinlock, then loop until the
+ * spinlock is successfully locked.
+ *
+ * This implementation is non-reentrant and set a bit of lock.
+ *
+ * The priority of reader is higher than writter if a reader hold the
+ * lock, a new reader can get its lock but writer can't get this lock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to lock.
+ *
+ * Returned Value:
+ * None. When the function returns, the spinlock was successfully locked
+ * by this CPU.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void read_lock(FAR volatile rwlock_t *lock);
+
+/****************************************************************************
+ * Name: read_unlock
+ *
+ * Description:
+ * Release a bit on a non-reentrant spinlock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to unlock.
+ *
+ * Returned Value:
+ * None.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void read_unlock(FAR volatile rwlock_t *lock);
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this CPU does not already hold the spinlock, then loop until the
+ * spinlock is successfully locked.
+ *
+ * This implementation is non-reentrant and set all bit on lock to avoid
+ * readers and writers.
+ *
+ * The priority of reader is higher than writter if a reader hold the
+ * lock, a new reader can get its lock but writer can't get this lock.
+ *
+ * Input Parameters:
+ * lock - A reference to the spinlock object to lock.
+ *
+ * Returned Value:
+ * None. When the function returns, the spinlock was successfully locked
+ * by this CPU.
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+void write_lock(FAR volatile rwlock_t *lock);
Review Comment:
let's add rwlock_init too
--
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
