Skip to content

Commit

Permalink
spinlock: remove recursive locks with write_lock_irqsave/read_lock_ir…
Browse files Browse the repository at this point in the history 
…qsave

reason:
1 There is a similar PR, apache#14079,
2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave.
3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance
4 Nested spinlock is also not available in Linux.
5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed.

Signed-off-by: hujun5 <[email protected]>
  • Loading branch information
@hujun260 @JaeheeKwon
hujun260 authored and JaeheeKwon committed Nov 28, 2024
1 parent 9751e35 commit 22cac19
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 123 deletions.
42 changes: 9 additions & 33 deletions include/nuttx/spinlock.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -995,25 +995,19 @@ static inline_function void write_unlock(FAR volatile rwlock_t *lock)
*
* Description:
* If SMP is 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,
* The argument lock should be 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
* or internal data structure) in SMP mode. 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
* lock - Caller specific spinlock, not NULL.
*
* Returned Value:
* An opaque, architecture-specific value that represents the state of
Expand All @@ -1032,19 +1026,15 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock);
*
* 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
* The argument lock should be 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.
* lock - Caller specific spinlock, not NULL.
*
* flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to read_lock_irqsave(lock);
Expand All @@ -1065,13 +1055,7 @@ void read_unlock_irqrestore(FAR rwlock_t *lock, irqstate_t flags);
*
* Description:
* If SMP is 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,
* The argument lock should be specified,
* disable local interrupts and take the lock spinlock and return
* the interrupt state.
*
Expand All @@ -1083,9 +1067,7 @@ void read_unlock_irqrestore(FAR rwlock_t *lock, irqstate_t flags);
* 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
* lock - Caller specific spinlock, not NULL.
*
* Returned Value:
* An opaque, architecture-specific value that represents the state of
Expand All @@ -1104,21 +1086,15 @@ irqstate_t write_lock_irqsave(FAR rwlock_t *lock);
*
* Description:
* If SMP is enabled:
* If the argument lock is not specified (i.e. NULL),
* decrement the call counter (g_irq_rw_spin_count[cpu]) and if it
* decrements to zero then release the spinlock (g_irq_rw_spin) and
* restore the interrupt state as it was prior to the previous call to
* write_lock_irqsave(NULL).
*
* If the argument lock is specified, release the lock and
* The argument lock should be specified, release the lock and
* restore the interrupt state as it was prior to the previous call to
* write_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.
* lock - Caller specific spinlock, not NULL.
*
* flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to write_lock_irqsave(lock);
Expand Down
102 changes: 12 additions & 90 deletions sched/irq/irq_spinlock.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,6 @@ volatile spinlock_t g_irq_spin = SP_UNLOCKED;

volatile uint8_t g_irq_spin_count[CONFIG_SMP_NCPUS];

#ifdef CONFIG_RW_SPINLOCK
/* Used for access control */

static volatile rwlock_t g_irq_rwspin = RW_SP_UNLOCKED;

/* Handles nested calls to write_lock_irqsave and write_unlock_irqrestore */

static volatile uint8_t g_irq_rwspin_count[CONFIG_SMP_NCPUS];

#endif

/****************************************************************************
* Public Functions
****************************************************************************/
Expand All @@ -69,11 +58,7 @@ static volatile uint8_t g_irq_rwspin_count[CONFIG_SMP_NCPUS];
*
* Description:
* If SMP is enabled:
* If the 'lock' argument is not specified (i.e. NULL), disable local
* interrupts and take the global read write spinlock (g_irq_rwspin)
* and increase g_irq_rwspin.
*
* If the 'lock' argument is specified,
* The argument lock should be specified,
* disable local interrupts and take the lock spinlock and return
* the interrupt state.
*
Expand All @@ -85,9 +70,7 @@ static volatile uint8_t g_irq_rwspin_count[CONFIG_SMP_NCPUS];
* 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
* lock - Caller specific spinlock, not NULL.
*
* Returned Value:
* An opaque, architecture-specific value that represents the state of
Expand All @@ -100,14 +83,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
irqstate_t ret;
ret = up_irq_save();

if (NULL == lock)
{
read_lock(&g_irq_rwspin);
}
else
{
read_lock(lock);
}
read_lock(lock);

return ret;
}
Expand All @@ -117,19 +93,15 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock)
*
* Description:
* If SMP is enabled:
* If the argument lock is not specified (i.e. NULL),
* decrement the call counter (g_irq_rwspin) 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
* The argument lock should be 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.
* lock - Caller specific spinlock, not NULL.
*
* flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to read_lock_irqsave(lock);
Expand All @@ -141,15 +113,7 @@ irqstate_t read_lock_irqsave(FAR rwlock_t *lock)

void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
{
if (NULL == lock)
{
read_unlock(&g_irq_rwspin);
}
else
{
read_unlock(lock);
}

read_unlock(lock);
up_irq_restore(flags);
}

Expand All @@ -158,13 +122,7 @@ void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
*
* Description:
* If SMP is enabled:
* If the argument lock is not specified (i.e. NULL),
* disable local interrupts and take the global spinlock (g_irq_rwspin)
* if the call counter (g_irq_rwspin_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,
* The argument lock should be specified,
* disable local interrupts and take the lock spinlock and return
* the interrupt state.
*
Expand All @@ -176,9 +134,7 @@ void read_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
* 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
* lock - Caller specific spinlock, not NULL.
*
* Returned Value:
* An opaque, architecture-specific value that represents the state of
Expand All @@ -191,21 +147,7 @@ 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_rwspin_count[me])
{
write_lock(&g_irq_rwspin);
}

g_irq_rwspin_count[me]++;
DEBUGASSERT(0 != g_irq_rwspin_count[me]);
}
else
{
write_lock(lock);
}
write_lock(lock);

return ret;
}
Expand All @@ -215,21 +157,15 @@ irqstate_t write_lock_irqsave(rwlock_t *lock)
*
* Description:
* If SMP is enabled:
* If the argument lock is not specified (i.e. NULL),
* decrement the call counter (g_irq_rwspin_count[cpu]) and if it
* decrements to zero then release the spinlock (g_irq_rwspin) and
* restore the interrupt state as it was prior to the previous call to
* write_lock_irqsave(NULL).
*
* If the argument lock is specified, release the lock and
* The argument lock should be specified, release the lock and
* restore the interrupt state as it was prior to the previous call to
* write_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.
* lock - Caller specific spinlock, not NULL.
*
* flags - The architecture-specific value that represents the state of
* the interrupts prior to the call to write_lock_irqsave(lock);
Expand All @@ -241,21 +177,7 @@ irqstate_t write_lock_irqsave(rwlock_t *lock)

void write_unlock_irqrestore(rwlock_t *lock, irqstate_t flags)
{
if (NULL == lock)
{
int me = this_cpu();
DEBUGASSERT(0 < g_irq_rwspin_count[me]);
g_irq_rwspin_count[me]--;

if (0 == g_irq_rwspin_count[me])
{
write_unlock(&g_irq_rwspin);
}
}
else
{
write_unlock(lock);
}
write_unlock(lock);

up_irq_restore(flags);
}
Expand Down

0 comments on commit 22cac19

Please sign in to comment.