You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/11/04 07:04:09 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new issue #2213: Implement the fine-grained spin lock
xiaoxiang781216 opened a new issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213
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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773934741
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773740788
>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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-731052528
After more thinking, we should support two type synchronization:
1. Multiple instance spinlock
2. Multiple instance critical section
The major difference is that the caller can't sleep afte hold the spinlock, but critical section can.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773826484
> > 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!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773826484
> > 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!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773938280
@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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-722160521
>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 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 for 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773826484
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773826484
> > 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!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-722163666
@masayuki2009 you are right. Let's introduce spinlock first and replace the usage step by step as note here: https://github.com/apache/incubator-nuttx/issues/1144.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 edited a comment on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-722160521
>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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #2213: Implement the fine-grained spin lock
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #2213:
URL: https://github.com/apache/incubator-nuttx/issues/2213#issuecomment-773740788
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org