[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #2213: Implement the fine-grained spin lock

[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