You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "TaiJuWu (via GitHub)" <gi...@apache.org> on 2023/09/22 13:52:26 UTC
[GitHub] [nuttx] TaiJuWu opened a new pull request, #10776: rw spinlock prototype
TaiJuWu opened a new pull request, #10776:
URL: https://github.com/apache/nuttx/pull/10776
## Summary
## Impact
## Testing
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
(error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid))
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
reference: https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02) [report](https://github.com/apache/nuttx/actions/runs/6436146710/job/17478987465)
```
Error: semaphore/spinlock.c:595:11: error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid)
while (!atomic_compare_exchange_strong(lock, &unlocked, \
^ ~~~~
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
Maybe there is a related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
Error: semaphore/spinlock.c:595:11: error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid)
while (!atomic_compare_exchange_strong(lock, &unlocked, \
^ ~~~~
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
Maybe there is a related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1358866963
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int old;
Review Comment:
@xiaoxiang781216 I want to splite two commits on this PR ?
One is the implementation of read-write spinlock and the other is for resolving Mac sim-02 compiler issue.
Maybe we can revert the second commit if resolving this compiler issue of Mac sim-02.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359878907
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ while (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_trylock
+ *
+ * Description:
+ * If this task 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool write_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
Review Comment:
I do't have any special reason.
The logical also can be modified to success case.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359889532
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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;
Review Comment:
since it makes the code more compact, and still confirm the c89 spec.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1343916842
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +454,120 @@ 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: check_bit
+ *
+ * Description:
+ * Check offset is set or not.
+ *
+ * Input Parameters:
+ * lock - A lock instance.
+ * offset - from right to left.
+ *
+ * Returned Value:
+ * bool
+ *
+ * Assumptions:
+ * No.
+ ****************************************************************************/
+
+inline bool check_bit(spinlock_t lock, uint8_t offset)
Review Comment:
This function is not needed so I deleted 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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1358866963
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int old;
Review Comment:
@xiaoxiang781216 Is it a good idea to splite two commit on this PR?
One is the implementation of read-write spinlock and the other is for resolving Mac sim-02 compiler issue.
Maybe we can revert the second commit if resolving this compiler issue of Mac sim-02.
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int old;
Review Comment:
@xiaoxiang781216 Is it a good idea to splite two commits on this PR?
One is the implementation of read-write spinlock and the other is for resolving Mac sim-02 compiler issue.
Maybe we can revert the second commit if resolving this compiler issue of Mac sim-02.
--
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] rw spinlock [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359142047
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
Review Comment:
```suggestion
* If SMP is enabled:
```
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
Review Comment:
see above
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
Review Comment:
```suggestion
* or internal data structure) in SMP mode. Do not use this API
```
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
Review Comment:
```suggestion
* If the 'lock' argument is not specified (i.e. NULL), disable local
```
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
Review Comment:
```suggestion
* If the 'lock' argument is specified,
```
##########
sched/Kconfig:
##########
@@ -318,6 +318,12 @@ config TICKET_SPINLOCK
endif # SPINLOCK
+config RW_SPINLOCK
+ bool "Support read-write Spinlocks"
+ default y
+ ---help---
+ Support Read-write spinlock
Review Comment:
Add complete help with the 'Why questions answered.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359881263
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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;
Review Comment:
Is there good reason to make this decision?
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359891808
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ while (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_trylock
+ *
+ * Description:
+ * If this task 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool write_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
Review Comment:
Actually if we don't include "arch/spinlock.h", SP_DSB, SP_DMB.... is useless and it will replace with empty line.
So we can remove 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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354294685
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
Align g_irq_spin_count.
If the lock is NULL, the caller will get global lock (e.g. g_irq_rw_spin)
And if the CPU can write lock, it can call write_lock_irqsave again (e.g. support nest).
Why is the variable called g_irq_write_spin_count?
Because reader can read data simultaneously and the counter is already record in lock.
Based on the reason, we only need to record the counter of the cpu get the write lock (nest).
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354294685
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
Align g_irq_spin_count.
If the lock is NULL, the caller will get global lock (e.g. g_irq_rw_spin) and spin_lock_irqsave() support nest on the same CPU.
If the CPU can write lock, it can call write_lock_irqsave() again (e.g. support nest).
Why is the variable called g_irq_write_spin_count?
Because reader can read data simultaneously and the counter is already record in lock.
Based on the reason, we only need to record the counter of the cpu get the write lock (nest).
But g_irq_rw_spin_count is still fine.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354144604
##########
sched/irq/irq_spinlock.c:
##########
@@ -45,6 +45,17 @@ static volatile spinlock_t g_irq_spin = SP_UNLOCKED;
static volatile uint8_t g_irq_spin_count[CONFIG_SMP_NCPUS];
+#ifdef CONFIG_RW_SPINLOCK
+/* Used for access control */
+
+static volatile rwlock_t g_irq_rw_spin = RW_SP_UNLOCKED;
+
+/* Handles nested calls to write_lock_irqsave and write_unlock_irqrestore */
+
+static volatile uint8_t g_irq_write_spin_count[CONFIG_SMP_NCPUS];
Review Comment:
```suggestion
static volatile uint8_t g_irq_rw_spin_count[CONFIG_SMP_NCPUS];
```
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +451,222 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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, &unlocked, \
+ RW_SP_WRITE_LOCKED))
Review Comment:
align
##########
sched/semaphore/spinlock.c:
##########
@@ -36,6 +36,11 @@
# include <stdatomic.h>
#endif
+#ifdef CONFIG_RW_SPINLOCK
+# include <stdatomic.h>
+const static rwlock_t unlocked = RW_SP_UNLOCKED;
Review Comment:
but, it's still better to define a local variable instead global.
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
why need g_irq_write_spin_count for write lock?
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +451,222 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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, &unlocked, \
Review Comment:
remove \
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
can we use the normal type to avoid include stdatomic.h in the header file?
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359892358
##########
sched/semaphore/spinlock.c:
##########
@@ -521,7 +521,7 @@ void read_lock(FAR volatile rwlock_t *lock)
bool read_trylock(FAR volatile rwlock_t *lock)
{
- rwlock_t old = atomic_load(lock);
+ int32_t old = atomic_load(lock);
Review Comment:
remove
##########
sched/semaphore/spinlock.c:
##########
@@ -596,7 +596,7 @@ void read_unlock(FAR volatile rwlock_t *lock)
void write_lock(FAR volatile rwlock_t *lock)
{
- rwlock_t zero = RW_SP_UNLOCKED;
+ int32_t zero = RW_SP_UNLOCKED;
Review Comment:
all int32_t to int
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
Error: semaphore/spinlock.c:484:48: error: incompatible pointer types passing 'rwlock_t *' (aka '_Atomic(int) *') to parameter of type 'int *' [-Werror,-Wincompatible-pointer-types]
[1306](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1307)
atomic_compare_exchange_strong(lock, &old, old + 1))
[1307](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1308)
^~~~```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
Maybe there is a related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
Error: semaphore/spinlock.c:484:48: error: incompatible pointer types passing 'rwlock_t *' (aka '_Atomic(int) *') to parameter of type 'int *' [-Werror,-Wincompatible-pointer-types]
[1306](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1307)
atomic_compare_exchange_strong(lock, &old, old + 1))
[1307](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1308)
^~~~
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
Maybe there is a related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1348747696
##########
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:
write_lock need to check rw_lock is equal zero and decide that writer can get this lock or not.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
(error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid))
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
Maybe there is a related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354294685
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
Align g_irq_spin_count.
If the lock is NULL, the caller will get global lock (e.g. g_irq_rw_spin)
And if the CPU can write lock, it can call write_lock_irqsave again (e.g. support nest).
Why is the variable called g_irq_write_spin_count?
Because reader can read data simultaneously and the counter is already record in lock.
Based on the reason, we only need to record the counter of the cpu get the write lock (nest).
But g_irq_rw_spin_count is still fine.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359495720
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359893853
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ while (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_trylock
+ *
+ * Description:
+ * If this task 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool write_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
Review Comment:
> so should we remove it?
Im not sure what is your it (line630 or line632).
But no matter SP_DSB(), SP_DMB()... or using success case, all answers are yes.
We can remove them.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359889589
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
Review Comment:
Yes, please go ahead.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1358866963
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int old;
Review Comment:
@xiaoxiang781216 I want to splite two commits on this PR ?
One is the implementation of read-write spinlock and the other is for resolving Mac sim-02 compiler issue.
Maybe we can revert the second commit if resolving this compiler issue of Mac sim-02.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359891808
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ while (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_trylock
+ *
+ * Description:
+ * If this task 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool write_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
Review Comment:
Actually if we don't include "arch/spinlock.h", SP_XXX is useless and it will replace with empty line.
So we can remove 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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359881979
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
Review Comment:
I want to modify
old < 0 ---> old <= RW_SP_WRITE_LOCKED
It's more readable.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1349168967
##########
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:
Done.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1348747696
##########
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:
write_lock need to check rw_lock is equal zero or not and decide that can writer get this 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
Re: [PR] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346732396
##########
include/nuttx/spinlock.h:
##########
@@ -51,6 +51,11 @@ typedef uint8_t spinlock_t;
#include <arch/spinlock.h>
+#if defined(CONFIG_RW_SPINLOCK)
Review Comment:
Done.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
(error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid))
```
So I used atomic type in the second commit of this PR but it seems not to work.
Does anyone help this part?
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
(error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid))
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
Error: semaphore/spinlock.c:595:11: error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid)
[1527](https://github.com/apache/nuttx/actions/runs/6436146710/job/17478987465#step:7:1528)
while (!atomic_compare_exchange_strong(lock, &unlocked
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
Maybe there is a related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359893853
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ while (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_trylock
+ *
+ * Description:
+ * If this task 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool write_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
Review Comment:
> so should we remove it?
Im not sure what is your it.
But no matter SP_DSB(), SP_DMB()... or using success case, all answers are yes.
We can remove them.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354649450
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
ok, let's fix the ci 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] rw spinlock prototype [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346789695
##########
include/nuttx/spinlock.h:
##########
@@ -285,7 +290,7 @@ void spin_unlock_wo_note(FAR volatile spinlock_t *lock);
****************************************************************************/
/* bool spin_islocked(FAR spinlock_t lock); */
-#define spin_islocked(l) (*(l) == SP_LOCKED)
+#define spin_islocked(l) (*(l) != SP_UNLOCKED)
Review Comment:
But, this is spinlock, not rw_spinlock. Please don't touch any code not related to rw_spinlock.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1347609262
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +388,138 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock)
+{
+ while (atomic_load(lock) == -1)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ atomic_fetch_add(lock, 1);
Review Comment:
Fixed.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346502131
##########
include/nuttx/spinlock.h:
##########
@@ -285,7 +290,7 @@ void spin_unlock_wo_note(FAR volatile spinlock_t *lock);
****************************************************************************/
/* bool spin_islocked(FAR spinlock_t lock); */
-#define spin_islocked(l) (*(l) == SP_LOCKED)
+#define spin_islocked(l) (*(l) != SP_UNLOCKED)
Review Comment:
Because rw_spinlock uses counter variable if the counter is set as 2 means there are two readers and the lock is locked.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1347041225
##########
include/nuttx/spinlock.h:
##########
@@ -285,7 +290,7 @@ void spin_unlock_wo_note(FAR volatile spinlock_t *lock);
****************************************************************************/
/* bool spin_islocked(FAR spinlock_t lock); */
-#define spin_islocked(l) (*(l) == SP_LOCKED)
+#define spin_islocked(l) (*(l) != SP_UNLOCKED)
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359587912
##########
sched/Kconfig:
##########
@@ -318,6 +318,12 @@ config TICKET_SPINLOCK
endif # SPINLOCK
+config RW_SPINLOCK
+ bool "Support read-write Spinlocks"
+ default y
+ ---help---
+ Support Read-write spinlock
Review Comment:
Done. Please review.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#issuecomment-1751203089
@xiaoxiang781216 Sorry, I have two questions.
First,
In current spin_lock_irqsave() and spin_lock_irqrestore(), it need to support global spinlock(g_irq_spin)?
Linux kernel seems not to support this feature.
Does read/write spinlock also need support this feature?
Second,
Why does spin_lock_irqrestore() need to support nested spinlock?
From [source code](https://github.com/TaiJuWu/nuttx/blob/921ae08e936d0bbdf7c056b21f8f5f5bf5f8dee1/sched/irq/irq_spinlock.c#L90), I can image a usage is
```
irqstate_t level1 = spin_lock_irqsave()
irqstate_t level2 = spin_lock_irqsave()
C.S
spin_lock_irqrestore(level2);
spin_lock_irqrestore(level1);
```
This is a little tricky for me and this feature is not used in kernel.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#issuecomment-1763269226
> > @TaiJuWu please squash the change into one patch.
>
> I prefer maintaining two commits of this PR. The reason is the first one commit is implementations of rw lock. The second is only for resolving CI issue(just for Mac sim-02). Maybe we can revert the commit after updating the compiler.
>
> How do you think?
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1358860001
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int old;
Review Comment:
In order to avoid compiler issue on Mac sim-02
[report](https://github.com/apache/nuttx/actions/runs/6485231606)
```
Error: semaphore/spinlock.c:630:45: error: incompatible pointer types passing 'rwlock_t *' (aka '_Atomic(int) *') to parameter of type 'int *' [-Werror,-Wincompatible-pointer-types]
if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
^~~~~
```
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359890835
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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;
Review Comment:
I remember C89 need to split declare and using.
It must be declare before using.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346731951
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +388,138 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock)
+{
+ while (atomic_load(lock) == -1)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ atomic_fetch_add(lock, 1);
Review Comment:
I already modify the implementation.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346505267
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +388,138 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock)
+{
+ while (atomic_load(lock) == -1)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ atomic_fetch_add(lock, 1);
Review Comment:
Yes. You're right.
I need to think how to implement this again.
--
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] rw spinlock prototype [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1343031216
##########
sched/semaphore/spinlock.c:
##########
@@ -24,6 +24,10 @@
#include <nuttx/config.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
Review Comment:
```suggestion
# include <stdatomic.h>
```
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +454,120 @@ 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: check_bit
+ *
+ * Description:
+ * Check offset is set or not.
+ *
+ * Input Parameters:
+ * lock - A lock instance.
+ * offset - from right to left.
+ *
+ * Returned Value:
+ * bool
+ *
+ * Assumptions:
+ * No.
+ ****************************************************************************/
+
+inline bool check_bit(spinlock_t lock, uint8_t offset)
+{
+ return lock & (1 << offset);
+}
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock);
Review Comment:
I think it is better to built-in `volatile` into `rw_spinlock_t`, but that is up to discussion.
##########
include/nuttx/spinlock.h:
##########
@@ -51,6 +51,11 @@ typedef uint8_t spinlock_t;
#include <arch/spinlock.h>
+#if defined(CONFIG_RW_SPINLOCK)
+typedef uint32_t rw_spinlock_t;
+#include <stdatomic.h>
Review Comment:
```suggestion
# include <stdatomic.h>
```
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +454,120 @@ 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: check_bit
+ *
+ * Description:
+ * Check offset is set or not.
+ *
+ * Input Parameters:
+ * lock - A lock instance.
+ * offset - from right to left.
+ *
+ * Returned Value:
+ * bool
+ *
+ * Assumptions:
+ * No.
+ ****************************************************************************/
+
+inline bool check_bit(spinlock_t lock, uint8_t offset)
Review Comment:
Can this be a macro?
--
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] rw spinlock prototype [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1348707455
##########
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:
but only read_lock need 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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346731951
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +388,138 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock)
+{
+ while (atomic_load(lock) == -1)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ atomic_fetch_add(lock, 1);
Review Comment:
I already modify the implementation.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354295219
##########
sched/semaphore/spinlock.c:
##########
@@ -36,6 +36,11 @@
# include <stdatomic.h>
#endif
+#ifdef CONFIG_RW_SPINLOCK
+# include <stdatomic.h>
+const static rwlock_t unlocked = RW_SP_UNLOCKED;
Review Comment:
Got 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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354294685
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
Align g_irq_spin_count.
If the lock is NULL, the caller will get global lock (e.g. g_irq_spin) and spin_lock_irqsave() support nest on the same CPU.
If the CPU can write lock, it can call write_lock_irqsave() again (e.g. support nest).
Why is the variable called g_irq_write_spin_count?
Because reader can read data simultaneously and the counter is already record in lock.
Based on the reason, we only need to record the counter of the cpu get the write lock (nest).
But g_irq_rw_spin_count is still fine.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #10776:
URL: https://github.com/apache/nuttx/pull/10776
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1343919334
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +454,120 @@ 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: check_bit
+ *
+ * Description:
+ * Check offset is set or not.
+ *
+ * Input Parameters:
+ * lock - A lock instance.
+ * offset - from right to left.
+ *
+ * Returned Value:
+ * bool
+ *
+ * Assumptions:
+ * No.
+ ****************************************************************************/
+
+inline bool check_bit(spinlock_t lock, uint8_t offset)
+{
+ return lock & (1 << offset);
+}
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock);
Review Comment:
I agree with you.
I just follow other function in this file.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359755758
##########
sched/irq/irq_spinlock.c:
##########
@@ -45,6 +45,17 @@ static volatile spinlock_t g_irq_spin = SP_UNLOCKED;
static volatile uint8_t g_irq_spin_count[CONFIG_SMP_NCPUS];
+#ifdef CONFIG_RW_SPINLOCK
+/* Used for access control */
+
+static volatile rwlock_t g_irq_rw_spin = RW_SP_UNLOCKED;
+
+/* Handles nested calls to write_lock_irqsave and write_unlock_irqrestore */
+
+static volatile uint8_t g_irq_write_spin_count[CONFIG_SMP_NCPUS];
Review Comment:
g_irq_rwspin_count
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ while (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_trylock
+ *
+ * Description:
+ * If this task 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool write_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
Review Comment:
why need in fail case
##########
sched/irq/irq_spinlock.c:
##########
@@ -45,6 +45,17 @@ static volatile spinlock_t g_irq_spin = SP_UNLOCKED;
static volatile uint8_t g_irq_spin_count[CONFIG_SMP_NCPUS];
+#ifdef CONFIG_RW_SPINLOCK
+/* Used for access control */
+
+static volatile rwlock_t g_irq_rw_spin = RW_SP_UNLOCKED;
Review Comment:
g_irq_rwspin
##########
sched/semaphore/spinlock.c:
##########
@@ -474,7 +474,7 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
void read_lock(FAR volatile rwlock_t *lock)
{
- rwlock_t old;
+ int32_t old;
Review Comment:
why not continue rwlock_t
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
Review Comment:
```
if (old < 0)
{
SP_DSB();
SP_WFE();
}
else if (atomic_compare_exchange_strong(lock, &old, old + 1))
{
break;
}
```
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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;
Review Comment:
merge to line 481
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
Review Comment:
```
while (true)
{
rwlock_t old = atomic_load(lock);
if (old < 0)
{
return false;
}
else if (atomic_compare_exchange_strong(lock, &old, old + 1))
{
break;
}
}
SP_DMB();
return true;
```
--
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] rw spinlock prototype [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346235964
##########
include/nuttx/spinlock.h:
##########
@@ -285,7 +290,7 @@ void spin_unlock_wo_note(FAR volatile spinlock_t *lock);
****************************************************************************/
/* bool spin_islocked(FAR spinlock_t lock); */
-#define spin_islocked(l) (*(l) == SP_LOCKED)
+#define spin_islocked(l) (*(l) != SP_UNLOCKED)
Review Comment:
why change this
##########
include/nuttx/spinlock.h:
##########
@@ -51,6 +51,11 @@ typedef uint8_t spinlock_t;
#include <arch/spinlock.h>
+#if defined(CONFIG_RW_SPINLOCK)
Review Comment:
where you define CONFIG_RW_SPINLOCK?
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +454,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: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock);
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_unlock(FAR volatile rw_spinlock_t *lock);
+
+/****************************************************************************
+ * Name: rw_wrt_spin_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 rw_wrt_spin_lock(FAR volatile rw_spinlock_t *lock);
+
+/****************************************************************************
+ * Name: rw_wrt_spin_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 rw_wrt_spin_unlock(FAR volatile rw_spinlock_t *lock);
Review Comment:
let's add irq and try version
##########
include/nuttx/spinlock.h:
##########
@@ -51,6 +51,11 @@ typedef uint8_t spinlock_t;
#include <arch/spinlock.h>
+#if defined(CONFIG_RW_SPINLOCK)
+typedef uint32_t rw_spinlock_t;
+# include <stdatomic.h>
Review Comment:
move the inclusion to source file
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +454,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: rw_read_spin_lock
Review Comment:
let's use the same name from https://elixir.bootlin.com/linux/latest/source/include/linux/rwlock.h:
rwlock_t, rwlock_init, read_trylock, write_trylock, read_lock, write_lock, read_unlock and write_unlock.
--
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] rw spinlock prototype [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346359981
##########
sched/semaphore/spinlock.c:
##########
@@ -384,4 +388,138 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#if defined(CONFIG_RW_SPINLOCK)
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock)
+{
+ while (atomic_load(lock) == -1)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ atomic_fetch_add(lock, 1);
Review Comment:
race since another writer may hold lock after line 419 and before line 425
--
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] rw spinlock prototype [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354294685
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
Align g_irq_spin_count.
If the lock is NULL, the caller will get glbal lock (e.g. g_irq_rw_spin)
And if the CPU can write lock, it can call write_lock_irqsave again (e.g. support nest).
Why is the variable called g_irq_write_spin_count?
Because reader can read data simultaneously and the counter is already record in lock.
Based on the reason, we only need to record the counter of the cpu get the write lock (nest).
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
(error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid))
```
So I used atomic type in the second commit of this PR but it seems not to work.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359587912
##########
sched/Kconfig:
##########
@@ -318,6 +318,12 @@ config TICKET_SPINLOCK
endif # SPINLOCK
+config RW_SPINLOCK
+ bool "Support read-write Spinlocks"
+ default y
+ ---help---
+ Support Read-write spinlock
Review Comment:
Done.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1358860001
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int old;
Review Comment:
In order to avoid compiler issue on Mac sim-02
[report](https://github.com/apache/nuttx/actions/runs/6485231606)
```
Error: semaphore/spinlock.c:630:45: error: incompatible pointer types passing 'rwlock_t *' (aka '_Atomic(int) *') to parameter of type 'int *' [-Werror,-Wincompatible-pointer-types]
[1446](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1447)
if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
[1447](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1448)
^~~~~
```
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1358860001
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int old;
Review Comment:
In order to avoid compiler issue on Mac sim-02
```
Error: semaphore/spinlock.c:630:45: error: incompatible pointer types passing 'rwlock_t *' (aka '_Atomic(int) *') to parameter of type 'int *' [-Werror,-Wincompatible-pointer-types]
[1446](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1447)
if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
[1447](https://github.com/apache/nuttx/actions/runs/6485231606/job/17611025912#step:7:1448)
^~~~~
```
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02) [report](https://github.com/apache/nuttx/actions/runs/6436146710/job/17478987465)
```
Error: semaphore/spinlock.c:595:11: error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid)
while (!atomic_compare_exchange_strong(lock, &unlocked, \
^ ~~~~
```
Maybe there is a related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#issuecomment-1763058232
@TaiJuWu please squash the change into one patch.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359878610
##########
sched/semaphore/spinlock.c:
##########
@@ -474,7 +474,7 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
void read_lock(FAR volatile rwlock_t *lock)
{
- rwlock_t old;
+ int32_t old;
Review Comment:
Just for pass Mac sim-02 ci
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1348687786
##########
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:
The variable is used at read_lock, write_lock,read_trylock....
In many case, we will use it so I think it should be static global variable.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354294685
##########
sched/irq/irq_spinlock.c:
##########
@@ -209,4 +220,202 @@ void spin_unlock_irqrestore_wo_note(spinlock_t *lock, irqstate_t flags)
up_irq_restore(flags);
}
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL), disable local
+ * interrupts and take the global read write spinlock (g_irq_rw_spin)
+ * and increase g_irq_rw_spin.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ read_lock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_lock(lock);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: read_unlock_irqrestore
+ *
+ * Description:
+ * If SMP is enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * decrement the call counter (g_irq_rw_spin) and restore the interrupt
+ * state as it was prior to the previous call to read_lock_irqsave(NULL).
+ *
+ * If the argument lock is specified, release the lock and
+ * restore the interrupt state as it was prior to the previous call to
+ * read_lock_irqsave(lock).
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_restore().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used.
+ *
+ * flags - The architecture-specific value that represents the state of
+ * the interrupts prior to the call to read_lock_irqsave(lock);
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
+{
+ if (NULL == lock)
+ {
+ read_unlock(&g_irq_rw_spin);
+ }
+ else
+ {
+ read_unlock(lock);
+ }
+
+ up_irq_restore(flags);
+}
+
+/****************************************************************************
+ * Name: write_lock_irqsave
+ *
+ * Description:
+ * If SMP is are enabled:
+ * If the argument lock is not specified (i.e. NULL),
+ * disable local interrupts and take the global spinlock (g_irq_rw_spin)
+ * if the call counter (g_irq_write_spin_count[cpu]) equals to 0. Then
+ * the counter on the CPU is incremented to allow nested calls and return
+ * the interrupt state.
+ *
+ * If the argument lock is specified,
+ * disable local interrupts and take the lock spinlock and return
+ * the interrupt state.
+ *
+ * NOTE: This API is very simple to protect data (e.g. H/W register
+ * or internal data structure) in SMP mode. But do not use this API
+ * with kernel APIs which suspend a caller thread. (e.g. nxsem_wait)
+ *
+ * If SMP is not enabled:
+ * This function is equivalent to up_irq_save().
+ *
+ * Input Parameters:
+ * lock - Caller specific spinlock. If specified NULL, g_irq_spin is used
+ * and can be nested. Otherwise, nested call for the same lock
+ * would cause a deadlock
+ *
+ * Returned Value:
+ * An opaque, architecture-specific value that represents the state of
+ * the interrupts prior to the call to write_lock_irqsave(lock);
+ *
+ ****************************************************************************/
+
+irqstate_t write_lock_irqsave(rwlock_t *lock)
+{
+ irqstate_t ret;
+ ret = up_irq_save();
+
+ if (NULL == lock)
+ {
+ int me = this_cpu();
+ if (0 == g_irq_write_spin_count[me])
Review Comment:
Align g_irq_spin_count.
If the lock is NULL, the caller will get global lock (e.g. g_irq_spin) and spin_lock_irqsave() support nest on the same CPU.
If the CPU can write lock, it can call write_lock_irqsave() again (e.g. support nest).
Why is the variable called g_irq_write_spin_count?
Because reader can read data simultaneously and the counter is already record in lock.
So we only need to record the counter of the cpu get the write lock (nest).
But g_irq_rw_spin_count is still fine.
--
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] rw spinlock prototype [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#issuecomment-1743535780
@xiaoxiang781216 what you think? What should be a good method to test to validate rw spinlock ?
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359890167
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,234 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ int32_t old;
+
+ while (true)
+ {
+ old = atomic_load(lock);
+
+ if (old <= RW_SP_WRITE_LOCKED)
+ {
+ DEBUGASSERT(old == RW_SP_WRITE_LOCKED);
+ SP_DSB();
+ SP_WFE();
+ }
+ else if(atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ int32_t old = atomic_load(lock);
+
+ while (true)
+ {
+ old = atomic_load(lock);
Review Comment:
merge line 524 here
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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 >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ break;
+ }
+ else if (old < 0)
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: read_trylock
+ *
+ * Description:
+ * If this task does not already hold the spinlock, then try to get the
+ * lock.
+ *
+ * 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool read_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t old = atomic_load(lock);
+
+ if (old >= RW_SP_UNLOCKED &&
+ atomic_compare_exchange_strong(lock, &old, old + 1))
+ {
+ SP_DMB();
+ return true;
+ }
+
+ return false;
+}
+
+/****************************************************************************
+ * 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) >= RW_SP_READ_LOCKED);
+
+ SP_DMB();
+ atomic_fetch_add(lock, -1);
+ SP_DSB();
+ SP_SEV();
+}
+
+/****************************************************************************
+ * Name: write_lock
+ *
+ * Description:
+ * If this task 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)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ while (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
+ SP_WFE();
+ }
+
+ SP_DMB();
+}
+
+/****************************************************************************
+ * Name: write_trylock
+ *
+ * Description:
+ * If this task 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:
+ * false - Failure, the spinlock was already locked
+ * true - Success, the spinlock was successfully locked
+ *
+ * Assumptions:
+ * Not running at the interrupt level.
+ *
+ ****************************************************************************/
+
+bool write_trylock(FAR volatile rwlock_t *lock)
+{
+ rwlock_t zero = RW_SP_UNLOCKED;
+
+ if (!atomic_compare_exchange_strong(lock, &zero, RW_SP_WRITE_LOCKED))
+ {
+ SP_DSB();
Review Comment:
so should we remove it?
##########
sched/semaphore/spinlock.c:
##########
@@ -474,7 +474,7 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
void read_lock(FAR volatile rwlock_t *lock)
{
- rwlock_t old;
+ int32_t old;
Review Comment:
ok, should we use int instead int32_t and please merge with line 481
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359892040
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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;
Review Comment:
no, c89 allow declare and assign at the same line, but forbid the declaration in the middle of statements.
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1359892463
##########
sched/semaphore/spinlock.c:
##########
@@ -446,4 +446,225 @@ void spin_clrbit(FAR volatile cpu_set_t *set, unsigned int cpu,
}
#endif
+#ifdef CONFIG_RW_SPINLOCK
+
+/****************************************************************************
+ * Name: read_lock
+ *
+ * Description:
+ * If this task 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;
Review Comment:
Ok. I need to some time to rewrite in order to split this PR into two commits.
--
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] rw spinlock [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#issuecomment-1763410835
please fix the style issue, @TaiJuWu .
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1354283729
##########
include/nuttx/spinlock.h:
##########
@@ -32,6 +32,14 @@
#include <nuttx/irq.h>
+#ifdef CONFIG_RW_SPINLOCK
+#include <stdatomic.h>
+typedef atomic_int rwlock_t;
Review Comment:
It can't.
There will be a error if we use normal type on Build / macOS (sim-02)
```
(error: address argument to atomic operation must be a pointer to _Atomic type ('volatile rwlock_t *' (aka 'volatile int *') invalid))
```
So I used atomic type in the second commit of this PR but it seems not to work.
Can anyone help this part?
Maybe there is related issue [here](https://lists.gnu.org/archive/html/bug-guile/2017-03/msg00084.html).
--
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] rw spinlock [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#issuecomment-1763102461
> @TaiJuWu please squash the change into one patch.
I prefer to maintain two commits of this PR.
The reason is the first one commit is implementations of rw lock.
The second is only for resolving CI issue and just for Mac sim-02.
Maybe we can revert the commit after updating the compiler.
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1343919334
##########
include/nuttx/spinlock.h:
##########
@@ -449,4 +454,120 @@ 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: check_bit
+ *
+ * Description:
+ * Check offset is set or not.
+ *
+ * Input Parameters:
+ * lock - A lock instance.
+ * offset - from right to left.
+ *
+ * Returned Value:
+ * bool
+ *
+ * Assumptions:
+ * No.
+ ****************************************************************************/
+
+inline bool check_bit(spinlock_t lock, uint8_t offset)
+{
+ return lock & (1 << offset);
+}
+
+/****************************************************************************
+ * Name: rw_read_spin_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 rw_read_spin_lock(FAR volatile rw_spinlock_t *lock);
Review Comment:
I agree with you.
I just follow other function in this file like [here](https://github.com/apache/nuttx/blob/94fa50315c237afa3a6b0084469e6e3fb5226547/include/nuttx/spinlock.h#L162).
--
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] rw spinlock prototype [nuttx]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #10776:
URL: https://github.com/apache/nuttx/pull/10776#discussion_r1346500501
##########
include/nuttx/spinlock.h:
##########
@@ -51,6 +51,11 @@ typedef uint8_t spinlock_t;
#include <arch/spinlock.h>
+#if defined(CONFIG_RW_SPINLOCK)
Review Comment:
It should be added to Kconfig but I didn't do that.
--
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