Implement the fine-grained spin lock #2213

xiaoxiang781216 · 2020-11-04T07:03:58Z

Right now, NuttX support one big spin lock:

irqstate_t spin_lock_irqsave(void);
void spin_unlock_irqrestore(irqstate_t flags);

Which isn't good for SMP machine. Compare with Linux(https://elixir.bootlin.com/linux/latest/source/include/linux/irqflags.h#L200), the above API should rename to:

irqstate_t local_irq_irqsave(void);
void local_irqrestore(irqstate_t flags);

and add the real spinlock(multiple instance) support:
https://www.kernel.org/doc/Documentation/locking/spinlocks.txt

The text was updated successfully, but these errors were encountered:

masayuki2009 · 2020-11-05T05:58:10Z

Right now, NuttX support one big spin lock:

@xiaoxiang781216
NuttX has several spinlocks (e.g., g_cpu_irqlock, g_cpu_sched_lock, ... ) inside the kernel.

However, yes, the current spin_lock_irqsave() and spin_unlock_irqrestore() uses a global spinlock named g_irq_spin. The reason why I introduced these APIs instead of multiple spinlock instance is to improve performance with minimum efforts just by replacing some critical section APIs where we do not need to block. (e.g. just protecting register access). Also, the APIs are compatible with non-SMP kernel.

the above API should rename to:

irqstate_t local_irq_irqsave(void);
void local_irqrestore(irqstate_t flags);

In my understanding, these APIs are the same as the current up_irq_save() and up_irq_restore() and also these APIs do not use a spinlock while spin_lock_irqsave() and spin_unlock_irqrestore() use a spinlock. So the behaviours are different.

and add the real spinlock(multiple instance) support:

I agree with you.

xiaoxiang781216 · 2020-11-05T06:06:44Z

@masayuki2009 you are right. Let's introduce spinlock first and replace the usage step by step as note here: #1144.

xiaoxiang781216 · 2020-11-20T09:26:05Z

After more thinking, we should support two type synchronization:

Multiple instance spinlock
Multiple instance critical section

The major difference is that the caller can't sleep afte hold the spinlock, but critical section can.

masayuki2009 · 2021-02-05T02:32:47Z

After more thinking, we should support two type synchronization:

Multiple instance spinlock

@xiaoxiang781216

I've just started this work by enhancing the existing APIs.
My idea is simple by just passing a spinlock

#if defined(CONFIG_SMP) && defined(CONFIG_SPINLOCK_IRQ)
-irqstate_t spin_lock_irqsave(void);
+irqstate_t spin_lock_irqsave(spinlock_t *lock);
 #else
-#  define spin_lock_irqsave() enter_critical_section()
+#  define spin_lock_irqsave(l) enter_critical_section()
 #endif

#if defined(CONFIG_SMP) && defined(CONFIG_SPINLOCK_IRQ)
-void spin_unlock_irqrestore(irqstate_t flags);
+void spin_unlock_irqrestore(spinlock_t *lock, irqstate_t flags);
 #else
-#  define spin_unlock_irqrestore(f) leave_critical_section(f)
+#  define spin_unlock_irqrestore(l, f) leave_critical_section(f)

Here, if the spinlock is NULL, it uses the global spinlock (i.e. g_irq_spin) for SMP.
And for a non-SMP case, it should be NULL as well.

For example, the imxrt does not support SMP, so imxrt_serial.c will have

flags  = spin_lock_irqsave(NULL);
...
spin_unlock_irqrestore(NULL, flags);

The behavior is the same as before. (i.e. It just disable/enable CPU interrupt for non-SMP case)

For SMP cases, you can also use the above call (i.e. spinlock is NULL). In this case, the behavior is also the same as before. And this will be useful for migration. For the first step, I will do this by just replacing the existing API call with the NULL spinlock.

Next step, some drivers would have a local spinlock for SMP, for example, cxd56_serial.c would be something like

--- a/arch/arm/src/cxd56xx/cxd56_serial.c
+++ b/arch/arm/src/cxd56xx/cxd56_serial.c
@@ -85,6 +85,9 @@ struct up_dev_s
   bool dtrdir;        /* DTR pin is the direction bit */
 #endif
   void *pmhandle;
+#ifdef CONFIG_SMP
+  spinlock_t lock;
+#endif
 };

...

   .oflow     = false, /* flow control is not supported */
 #endif
+#ifdef CONFIG_SMP
+  .lock      = SP_UNLOCKED,
+#endif
 };

static uart_dev_t g_uart2port =
@@ -288,7 +297,7 @@ static inline void up_disableuartint(FAR struct up_dev_s *priv,
 {
   irqstate_t flags;
 
-  flags = spin_lock_irqsave();
+  flags = spin_lock_irqsave(&priv->lock);
   if (ier)
     {
       *ier = priv->ier & UART_INTR_ALL;
@@ -296,7 +305,7 @@ static inline void up_disableuartint(FAR struct up_dev_s *priv,
 
   priv->ier &= ~UART_INTR_ALL;
   up_serialout(priv, CXD56_UART_IMSC, priv->ier);
-  spin_unlock_irqrestore(flags);
+  spin_unlock_irqrestore(&priv->lock, flags);
 }

In this case, each UART will have a spinlock but only for the SMP case. For the non-SMP case, the spinlock is not allocated but it's OK because the parameter (&priv->lock) will be ignored in the macro.

What do you think?

xiaoxiang781216 · 2021-02-05T06:35:45Z

After more thinking, we should support two type synchronization:
Multiple instance spinlock

@xiaoxiang781216

I've just started this work by enhancing the existing APIs.
My idea is simple by just passing a spinlock
#if defined(CONFIG_SMP) && defined(CONFIG_SPINLOCK_IRQ)
-irqstate_t spin_lock_irqsave(void);
+irqstate_t spin_lock_irqsave(spinlock_t *lock);
 #else
-#  define spin_lock_irqsave() enter_critical_section()
+#  define spin_lock_irqsave(l) enter_critical_section()

It's better change to:

#  define spin_lock_irqsave(l) up_irq_save()

#endif

#if defined(CONFIG_SMP) && defined(CONFIG_SPINLOCK_IRQ)
-void spin_unlock_irqrestore(irqstate_t flags);
+void spin_unlock_irqrestore(spinlock_t *lock, irqstate_t flags);
#else
-# define spin_unlock_irqrestore(f) leave_critical_section(f)
+# define spin_unlock_irqrestore(l, f) leave_critical_section(f)

It's better change to:

#  define spin_unlock_irqrestore(l, f) up_irq_restore(f)

Here, if the spinlock is NULL, it uses the global spinlock (i.e. g_irq_spin) for SMP.
And for a non-SMP case, it should be NULL as well.

For example, the imxrt does not support SMP, so imxrt_serial.c will have
flags  = spin_lock_irqsave(NULL);
...
spin_unlock_irqrestore(NULL, flags);
The behavior is the same as before. (i.e. It just disable/enable CPU interrupt for non-SMP case)

For SMP cases, you can also use the above call (i.e. spinlock is NULL). In this case, the behavior is also the same as before. And this will be useful for migration. For the first step, I will do this by just replacing the existing API call with the NULL spinlock.

Yes, it is a good compromise.

Next step, some drivers would have a local spinlock for SMP, for example, cxd56_serial.c would be something like

--- a/arch/arm/src/cxd56xx/cxd56_serial.c
+++ b/arch/arm/src/cxd56xx/cxd56_serial.c
@@ -85,6 +85,9 @@ struct up_dev_s
   bool dtrdir;        /* DTR pin is the direction bit */
 #endif
   void *pmhandle;
+#ifdef CONFIG_SMP
+  spinlock_t lock;
+#endif
 };

...

   .oflow     = false, /* flow control is not supported */
 #endif
+#ifdef CONFIG_SMP
+  .lock      = SP_UNLOCKED,
+#endif
 };

Can we add some macros for definition and initialization? so we can avoid spread #ifded/#endif in the whole code base.

static uart_dev_t g_uart2port =
@@ -288,7 +297,7 @@ static inline void up_disableuartint(FAR struct up_dev_s *priv,
{
irqstate_t flags;

flags = spin_lock_irqsave();

flags = spin_lock_irqsave(&priv->lock);
if (ier)
{
*ier = priv->ier & UART_INTR_ALL;
@@ -296,7 +305,7 @@ static inline void up_disableuartint(FAR struct up_dev_s *priv,

priv->ier &= ~UART_INTR_ALL;
up_serialout(priv, CXD56_UART_IMSC, priv->ier);

spin_unlock_irqrestore(flags);

spin_unlock_irqrestore(&priv->lock, flags);
}
In this case, each UART will have a spinlock but only for the SMP case. For the non-SMP case, the spinlock is not allocated but it's OK because the parameter `(&priv->lock)` will be ignored in the macro.

What do you think?

The propose looks great, thanks @masayuki2009!

masayuki2009 · 2021-02-05T09:53:29Z

@xiaoxiang781216

Thanks for your comments.

It's better change to:
# define spin_lock_irqsave(l) up_irq_save()

If we apply the above change, the behavior will be different in the case of CONFIG_SMP=y and CONFIG_SPINLOCK_IRQ=n.

#if defined(CONFIG_SMP) && defined(CONFIG_SPINLOCK_IRQ)
-irqstate_t spin_lock_irqsave(void);
+irqstate_t spin_lock_irqsave(spinlock_t *lock);
 #else
-#  define spin_lock_irqsave() enter_critical_section()
+# define spin_lock_irqsave(l) up_irq_save()

However, I think it's better to remove CONFIG_SPINLOCK_IRQ from Kconfig to avoid complexity.
In this case, finally, we get

#if defined(CONFIG_SMP) 
-irqstate_t spin_lock_irqsave(void);
+irqstate_t spin_lock_irqsave(spinlock_t *lock);
 #else
-#  define spin_lock_irqsave() enter_critical_section()
+# define spin_lock_irqsave(l) up_irq_save()

What do you think?

xiaoxiang781216 · 2021-02-05T10:10:22Z

Oh, I forgot SPINLOCK_IRQ config, but this option doesn't make sense for me, because the user can freely select:

spin_lock/spin_unlock to hold only spinlock or
spin_lock_irqsave/spin_unlock_irqrestore to hold both spinlock and irq

Two set API is already flexible enough for all use case.
On the other hand, it most likely generate synchronization error if user disable SPINLOCK_IRQ accidently in the current code base.

masayuki2009 · 2021-02-05T10:16:08Z

@xiaoxiang781216

Oh, I forgot SPINLOCK_IRQ config, but this option doesn't make sense for me, because the user can freely select:

I wrote the code but I agree with you.
OK, I will create a separate PR to remove CONFIG_SPINLOCK_IRQ from Kconfig.

xiaoxiang781216 · 2023-06-14T04:33:44Z

The fine grant spinlock is implemented, let's close it.

xiaoxiang781216 mentioned this issue Nov 4, 2020

Crtical section should be replaced with spinlock as much as we can to improve SMP performance #1144

Open

protobits added the enhancement New feature or request label Nov 4, 2020

This was referenced Feb 6, 2021

boards, include, sched: Remove CONFIG_SPINLOCK_IRQ #2816

Merged

arch, boards, drivers, include, sched, wireless: Change spinlock APIs. #2820

Merged

Driver-specific spinlock for cxd56_serial.c #2830

Merged

Ouss4 mentioned this issue Aug 24, 2021

Use driver-specific spinlock for ESP32 serial driver. #4403

Merged

Ouss4 mentioned this issue Sep 14, 2021

esp32: Use device specific lock as much as possible. #4543

Merged

xiaoxiang781216 mentioned this issue Dec 20, 2021

fs/vfs: Add file descriptor based timers support #5014

Merged

xiaoxiang781216 closed this as completed Jun 14, 2023

TaiJuWu mentioned this issue Oct 19, 2023

Question about enter_critical_section #10981

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the fine-grained spin lock #2213

Implement the fine-grained spin lock #2213

xiaoxiang781216 commented Nov 4, 2020

masayuki2009 commented Nov 5, 2020 •

edited

xiaoxiang781216 commented Nov 5, 2020

xiaoxiang781216 commented Nov 20, 2020

masayuki2009 commented Feb 5, 2021

xiaoxiang781216 commented Feb 5, 2021 •

edited

masayuki2009 commented Feb 5, 2021

xiaoxiang781216 commented Feb 5, 2021 •

edited

masayuki2009 commented Feb 5, 2021

xiaoxiang781216 commented Jun 14, 2023

Implement the fine-grained spin lock #2213

Implement the fine-grained spin lock #2213

Comments

xiaoxiang781216 commented Nov 4, 2020

masayuki2009 commented Nov 5, 2020 • edited

xiaoxiang781216 commented Nov 5, 2020

xiaoxiang781216 commented Nov 20, 2020

masayuki2009 commented Feb 5, 2021

xiaoxiang781216 commented Feb 5, 2021 • edited

masayuki2009 commented Feb 5, 2021

xiaoxiang781216 commented Feb 5, 2021 • edited

masayuki2009 commented Feb 5, 2021

xiaoxiang781216 commented Jun 14, 2023

masayuki2009 commented Nov 5, 2020 •

edited

xiaoxiang781216 commented Feb 5, 2021 •

edited

xiaoxiang781216 commented Feb 5, 2021 •

edited