Revert "mm/iob: Replace the critical section with spin lock"

GUIDINGLI · xiaoxiang781216 · commit b4def16ac371 · 2024-10-15T22:45:02.000+08:00
`g_iob_sem.semcount` is both manually changed in iob source code and api
nxsem_xxx.

nxsem related API uses critical_section to ensure sem value is modified
correctly. If iob using spin lock and modify sem value in the same time,
it's not safe.

This PR revert the spin lock change and uses critical section to align
with what nxsem uses.
diff --git a/mm/iob/iob.h b/mm/iob/iob.h
@@ -32,7 +32,6 @@
 #include <debug.h>
 
 #include <nuttx/mm/iob.h>
-#include <nuttx/spinlock.h>
 #include <nuttx/semaphore.h>
 
 #ifdef CONFIG_MM_IOB
@@ -85,8 +84,6 @@ extern sem_t g_throttle_sem;  /* Counts available I/O buffers when throttled */
 extern sem_t g_qentry_sem;    /* Counts free I/O buffer queue containers */
 #endif
 
-extern spinlock_t g_iob_lock;
-
 /****************************************************************************
  * Public Function Prototypes
  ****************************************************************************/
diff --git a/mm/iob/iob_add_queue.c b/mm/iob/iob_add_queue.c
@@ -63,7 +63,7 @@ static int iob_add_queue_internal(FAR struct iob_s *iob,
 
   qentry->qe_flink = NULL;
 
-  irqstate_t flags = spin_lock_irqsave(&g_iob_lock);
+  irqstate_t flags = enter_critical_section();
   if (!iobq->qh_head)
     {
       iobq->qh_head = qentry;
@@ -76,7 +76,7 @@ static int iob_add_queue_internal(FAR struct iob_s *iob,
       iobq->qh_tail = qentry;
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
+  leave_critical_section(flags);
 
   return 0;
 }
diff --git a/mm/iob/iob_alloc.c b/mm/iob/iob_alloc.c
@@ -78,7 +78,7 @@ static FAR struct iob_s *iob_alloc_committed(void)
    * to protect the committed list:  We disable interrupts very briefly.
    */
 
-  flags = spin_lock_irqsave(&g_iob_lock);
+  flags = enter_critical_section();
 
   /* Take the I/O buffer from the head of the committed list */
 
@@ -97,7 +97,7 @@ static FAR struct iob_s *iob_alloc_committed(void)
       iob->io_pktlen = 0;    /* Total length of the packet */
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
+  leave_critical_section(flags);
   return iob;
 }
 
@@ -264,7 +264,7 @@ FAR struct iob_s *iob_tryalloc(bool throttled)
    * to protect the free list:  We disable interrupts very briefly.
    */
 
-  flags = spin_lock_irqsave(&g_iob_lock);
+  flags = enter_critical_section();
 
 #if CONFIG_IOB_THROTTLE > 0
   /* If there are free I/O buffers for this allocation */
@@ -308,7 +308,7 @@ FAR struct iob_s *iob_tryalloc(bool throttled)
             }
 #endif
 
-          spin_unlock_irqrestore(&g_iob_lock, flags);
+          leave_critical_section(flags);
 
           /* Put the I/O buffer in a known state */
 
@@ -320,7 +320,7 @@ FAR struct iob_s *iob_tryalloc(bool throttled)
         }
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
+  leave_critical_section(flags);
   return NULL;
 }
 
diff --git a/mm/iob/iob_alloc_qentry.c b/mm/iob/iob_alloc_qentry.c
@@ -59,7 +59,7 @@ static FAR struct iob_qentry_s *iob_alloc_qcommitted(void)
    * to protect the committed list:  We disable interrupts very briefly.
    */
 
-  flags = spin_lock_irqsave(&g_iob_lock);
+  flags = enter_critical_section();
 
   /* Take the I/O buffer from the head of the committed list */
 
@@ -75,7 +75,7 @@ static FAR struct iob_qentry_s *iob_alloc_qcommitted(void)
       iobq->qe_head = NULL; /* Nothing is contained */
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
+  leave_critical_section(flags);
   return iobq;
 }
 
@@ -201,7 +201,7 @@ FAR struct iob_qentry_s *iob_tryalloc_qentry(void)
    * to protect the free list:  We disable interrupts very briefly.
    */
 
-  flags = spin_lock_irqsave(&g_iob_lock);
+  flags = enter_critical_section();
   iobq  = g_iob_freeqlist;
   if (iobq)
     {
@@ -227,7 +227,7 @@ FAR struct iob_qentry_s *iob_tryalloc_qentry(void)
       iobq->qe_head = NULL; /* Nothing is contained */
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
+  leave_critical_section(flags);
   return iobq;
 }
 
diff --git a/mm/iob/iob_free.c b/mm/iob/iob_free.c
@@ -135,7 +135,7 @@ FAR struct iob_s *iob_free(FAR struct iob_s *iob)
    * interrupts very briefly.
    */
 
-  flags = spin_lock_irqsave(&g_iob_lock);
+  flags = enter_critical_section();
 
   /* Which list?  If there is a task waiting for an IOB, then put
    * the IOB on either the free list or on the committed list where
@@ -168,7 +168,7 @@ FAR struct iob_s *iob_free(FAR struct iob_s *iob)
       g_iob_freelist  = iob;
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
+  leave_critical_section(flags);
 
   /* Signal that an IOB is available. This is done with schedule locked
    * to make sure that both g_iob_sem and g_throttle_sem are incremented
@@ -184,7 +184,7 @@ FAR struct iob_s *iob_free(FAR struct iob_s *iob)
   DEBUGASSERT(g_iob_sem.semcount <= CONFIG_IOB_NBUFFERS);
 
 #if CONFIG_IOB_THROTTLE > 0
-  flags = spin_lock_irqsave(&g_iob_lock);
+  flags = enter_critical_section();
 
   if (g_iob_sem.semcount > CONFIG_IOB_THROTTLE)
     {
@@ -205,15 +205,15 @@ FAR struct iob_s *iob_free(FAR struct iob_s *iob)
           g_iob_sem.semcount--;
         }
 
-      spin_unlock_irqrestore(&g_iob_lock, flags);
+      leave_critical_section(flags);
 
       nxsem_post(&g_throttle_sem);
       DEBUGASSERT(g_throttle_sem.semcount <=
               (CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE));
     }
   else
     {
-      spin_unlock_irqrestore(&g_iob_lock, flags);
+      leave_critical_section(flags);
     }
 #endif
 
diff --git a/mm/iob/iob_free_qentry.c b/mm/iob/iob_free_qentry.c
@@ -60,7 +60,7 @@ FAR struct iob_qentry_s *iob_free_qentry(FAR struct iob_qentry_s *iobq)
    * interrupts very briefly.
    */
 
-  flags = spin_lock_irqsave(&g_iob_lock);
+  flags = enter_critical_section();
 
   /* Which list?  If there is a task waiting for an IOB chain, then put
    * the IOB chain on either the free list or on the committed list where
@@ -79,8 +79,6 @@ FAR struct iob_qentry_s *iob_free_qentry(FAR struct iob_qentry_s *iobq)
       g_iob_freeqlist  = iobq;
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
-
   /* Signal that an I/O buffer chain container is available.  If there
    * is a thread waiting for an I/O buffer chain container, this will
    * wake up exactly one thread.  The semaphore count will correctly
@@ -89,6 +87,7 @@ FAR struct iob_qentry_s *iob_free_qentry(FAR struct iob_qentry_s *iobq)
    */
 
   nxsem_post(&g_qentry_sem);
+  leave_critical_section(flags);
 
   /* And return the I/O buffer chain container after the one that was freed */
 
diff --git a/mm/iob/iob_free_queue_qentry.c b/mm/iob/iob_free_queue_qentry.c
@@ -53,7 +53,7 @@ void iob_free_queue_qentry(FAR struct iob_s *iob,
   FAR struct iob_qentry_s *prev = NULL;
   FAR struct iob_qentry_s *qentry;
 
-  irqstate_t flags = spin_lock_irqsave(&g_iob_lock);
+  irqstate_t flags = enter_critical_section();
   for (qentry = iobq->qh_head; qentry != NULL;
        prev = qentry, qentry = qentry->qe_flink)
     {
@@ -75,8 +75,6 @@ void iob_free_queue_qentry(FAR struct iob_s *iob,
               iobq->qh_tail = prev;
             }
 
-          spin_unlock_irqrestore(&g_iob_lock, flags);
-
           /* Remove the queue container */
 
           iob_free_qentry(qentry);
@@ -85,11 +83,11 @@ void iob_free_queue_qentry(FAR struct iob_s *iob,
 
           iob_free_chain(iob);
 
-          return;
+          break;
         }
     }
 
-  spin_unlock_irqrestore(&g_iob_lock, flags);
+  leave_critical_section(flags);
 }
 
 #endif /* CONFIG_IOB_NCHAINS > 0 */
diff --git a/mm/iob/iob_initialize.c b/mm/iob/iob_initialize.c
@@ -108,8 +108,6 @@ sem_t g_throttle_sem = SEM_INITIALIZER(CONFIG_IOB_NBUFFERS -
 sem_t g_qentry_sem = SEM_INITIALIZER(CONFIG_IOB_NCHAINS);
 #endif
 
-spinlock_t g_iob_lock = SP_UNLOCKED;
-
 /****************************************************************************
  * Public Functions
  ****************************************************************************/
diff --git a/mm/iob/iob_remove_queue.c b/mm/iob/iob_remove_queue.c
@@ -58,7 +58,7 @@ FAR struct iob_s *iob_remove_queue(FAR struct iob_queue_s *iobq)
 
   /* Remove the I/O buffer chain from the head of the queue */
 
-  irqstate_t flags = spin_lock_irqsave(&g_iob_lock);
+  irqstate_t flags = enter_critical_section();
   qentry = iobq->qh_head;
   if (qentry)
     {
@@ -68,20 +68,15 @@ FAR struct iob_s *iob_remove_queue(FAR struct iob_queue_s *iobq)
           iobq->qh_tail = NULL;
         }
 
-      spin_unlock_irqrestore(&g_iob_lock, flags);
-
       /* Extract the I/O buffer chain from the container and free the
        * container.
        */
 
       iob = qentry->qe_head;
       iob_free_qentry(qentry);
     }
-  else
-    {
-      spin_unlock_irqrestore(&g_iob_lock, flags);
-    }
 
+  leave_critical_section(flags);
   return iob;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ static int iob_add_queue_internal(FAR struct iob_s *iob,`
`63`	`63`
`64`	`64`	`qentry->qe_flink = NULL;`
`65`	`65`
`66`		`- irqstate_t flags = spin_lock_irqsave(&g_iob_lock);`
	`66`	`+ irqstate_t flags = enter_critical_section();`
`67`	`67`	`if (!iobq->qh_head)`
`68`	`68`	`{`
`69`	`69`	`iobq->qh_head = qentry;`
`@@ -76,7 +76,7 @@ static int iob_add_queue_internal(FAR struct iob_s *iob,`
`76`	`76`	`iobq->qh_tail = qentry;`
`77`	`77`	`}`
`78`	`78`
`79`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`79`	`+ leave_critical_section(flags);`
`80`	`80`
`81`	`81`	`return 0;`
`82`	`82`	`}`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ static FAR struct iob_s *iob_alloc_committed(void)`
`78`	`78`	`* to protect the committed list: We disable interrupts very briefly.`
`79`	`79`	`*/`
`80`	`80`
`81`		`- flags = spin_lock_irqsave(&g_iob_lock);`
	`81`	`+ flags = enter_critical_section();`
`82`	`82`
`83`	`83`	`/* Take the I/O buffer from the head of the committed list */`
`84`	`84`
`@@ -97,7 +97,7 @@ static FAR struct iob_s *iob_alloc_committed(void)`
`97`	`97`	`iob->io_pktlen = 0; /* Total length of the packet */`
`98`	`98`	`}`
`99`	`99`
`100`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`100`	`+ leave_critical_section(flags);`
`101`	`101`	`return iob;`
`102`	`102`	`}`
`103`	`103`
`@@ -264,7 +264,7 @@ FAR struct iob_s *iob_tryalloc(bool throttled)`
`264`	`264`	`* to protect the free list: We disable interrupts very briefly.`
`265`	`265`	`*/`
`266`	`266`
`267`		`- flags = spin_lock_irqsave(&g_iob_lock);`
	`267`	`+ flags = enter_critical_section();`
`268`	`268`
`269`	`269`	`#if CONFIG_IOB_THROTTLE > 0`
`270`	`270`	`/* If there are free I/O buffers for this allocation */`
`@@ -308,7 +308,7 @@ FAR struct iob_s *iob_tryalloc(bool throttled)`
`308`	`308`	`}`
`309`	`309`	`#endif`
`310`	`310`
`311`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`311`	`+ leave_critical_section(flags);`
`312`	`312`
`313`	`313`	`/* Put the I/O buffer in a known state */`
`314`	`314`
`@@ -320,7 +320,7 @@ FAR struct iob_s *iob_tryalloc(bool throttled)`
`320`	`320`	`}`
`321`	`321`	`}`
`322`	`322`
`323`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`323`	`+ leave_critical_section(flags);`
`324`	`324`	`return NULL;`
`325`	`325`	`}`
`326`	`326`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ static FAR struct iob_qentry_s *iob_alloc_qcommitted(void)`
`59`	`59`	`* to protect the committed list: We disable interrupts very briefly.`
`60`	`60`	`*/`
`61`	`61`
`62`		`- flags = spin_lock_irqsave(&g_iob_lock);`
	`62`	`+ flags = enter_critical_section();`
`63`	`63`
`64`	`64`	`/* Take the I/O buffer from the head of the committed list */`
`65`	`65`
`@@ -75,7 +75,7 @@ static FAR struct iob_qentry_s *iob_alloc_qcommitted(void)`
`75`	`75`	`iobq->qe_head = NULL; /* Nothing is contained */`
`76`	`76`	`}`
`77`	`77`
`78`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`78`	`+ leave_critical_section(flags);`
`79`	`79`	`return iobq;`
`80`	`80`	`}`
`81`	`81`
`@@ -201,7 +201,7 @@ FAR struct iob_qentry_s *iob_tryalloc_qentry(void)`
`201`	`201`	`* to protect the free list: We disable interrupts very briefly.`
`202`	`202`	`*/`
`203`	`203`
`204`		`- flags = spin_lock_irqsave(&g_iob_lock);`
	`204`	`+ flags = enter_critical_section();`
`205`	`205`	`iobq = g_iob_freeqlist;`
`206`	`206`	`if (iobq)`
`207`	`207`	`{`
`@@ -227,7 +227,7 @@ FAR struct iob_qentry_s *iob_tryalloc_qentry(void)`
`227`	`227`	`iobq->qe_head = NULL; /* Nothing is contained */`
`228`	`228`	`}`
`229`	`229`
`230`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`230`	`+ leave_critical_section(flags);`
`231`	`231`	`return iobq;`
`232`	`232`	`}`
`233`	`233`
Original file line number	Diff line number	Diff line change
`@@ -135,7 +135,7 @@ FAR struct iob_s iob_free(FAR struct iob_s iob)`
`135`	`135`	`* interrupts very briefly.`
`136`	`136`	`*/`
`137`	`137`
`138`		`- flags = spin_lock_irqsave(&g_iob_lock);`
	`138`	`+ flags = enter_critical_section();`
`139`	`139`
`140`	`140`	`/* Which list? If there is a task waiting for an IOB, then put`
`141`	`141`	`* the IOB on either the free list or on the committed list where`
`@@ -168,7 +168,7 @@ FAR struct iob_s iob_free(FAR struct iob_s iob)`
`168`	`168`	`g_iob_freelist = iob;`
`169`	`169`	`}`
`170`	`170`
`171`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`171`	`+ leave_critical_section(flags);`
`172`	`172`
`173`	`173`	`/* Signal that an IOB is available. This is done with schedule locked`
`174`	`174`	`* to make sure that both g_iob_sem and g_throttle_sem are incremented`
`@@ -184,7 +184,7 @@ FAR struct iob_s iob_free(FAR struct iob_s iob)`
`184`	`184`	`DEBUGASSERT(g_iob_sem.semcount <= CONFIG_IOB_NBUFFERS);`
`185`	`185`
`186`	`186`	`#if CONFIG_IOB_THROTTLE > 0`
`187`		`- flags = spin_lock_irqsave(&g_iob_lock);`
	`187`	`+ flags = enter_critical_section();`
`188`	`188`
`189`	`189`	`if (g_iob_sem.semcount > CONFIG_IOB_THROTTLE)`
`190`	`190`	`{`
`@@ -205,15 +205,15 @@ FAR struct iob_s iob_free(FAR struct iob_s iob)`
`205`	`205`	`g_iob_sem.semcount--;`
`206`	`206`	`}`
`207`	`207`
`208`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`208`	`+ leave_critical_section(flags);`
`209`	`209`
`210`	`210`	`nxsem_post(&g_throttle_sem);`
`211`	`211`	`DEBUGASSERT(g_throttle_sem.semcount <=`
`212`	`212`	`(CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE));`
`213`	`213`	`}`
`214`	`214`	`else`
`215`	`215`	`{`
`216`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`216`	`+ leave_critical_section(flags);`
`217`	`217`	`}`
`218`	`218`	`#endif`
`219`	`219`
Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ void iob_free_queue_qentry(FAR struct iob_s *iob,`
`53`	`53`	`FAR struct iob_qentry_s *prev = NULL;`
`54`	`54`	`FAR struct iob_qentry_s *qentry;`
`55`	`55`
`56`		`- irqstate_t flags = spin_lock_irqsave(&g_iob_lock);`
	`56`	`+ irqstate_t flags = enter_critical_section();`
`57`	`57`	`for (qentry = iobq->qh_head; qentry != NULL;`
`58`	`58`	`prev = qentry, qentry = qentry->qe_flink)`
`59`	`59`	`{`
`@@ -75,8 +75,6 @@ void iob_free_queue_qentry(FAR struct iob_s *iob,`
`75`	`75`	`iobq->qh_tail = prev;`
`76`	`76`	`}`
`77`	`77`
`78`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
`79`		`-`
`80`	`78`	`/* Remove the queue container */`
`81`	`79`
`82`	`80`	`iob_free_qentry(qentry);`
`@@ -85,11 +83,11 @@ void iob_free_queue_qentry(FAR struct iob_s *iob,`
`85`	`83`
`86`	`84`	`iob_free_chain(iob);`
`87`	`85`
`88`		`- return;`
	`86`	`+ break;`
`89`	`87`	`}`
`90`	`88`	`}`
`91`	`89`
`92`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
	`90`	`+ leave_critical_section(flags);`
`93`	`91`	`}`
`94`	`92`
`95`	`93`	`#endif /* CONFIG_IOB_NCHAINS > 0 */`
Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,7 @@ FAR struct iob_s iob_remove_queue(FAR struct iob_queue_s iobq)`
`58`	`58`
`59`	`59`	`/* Remove the I/O buffer chain from the head of the queue */`
`60`	`60`
`61`		`- irqstate_t flags = spin_lock_irqsave(&g_iob_lock);`
	`61`	`+ irqstate_t flags = enter_critical_section();`
`62`	`62`	`qentry = iobq->qh_head;`
`63`	`63`	`if (qentry)`
`64`	`64`	`{`
`@@ -68,20 +68,15 @@ FAR struct iob_s iob_remove_queue(FAR struct iob_queue_s iobq)`
`68`	`68`	`iobq->qh_tail = NULL;`
`69`	`69`	`}`
`70`	`70`
`71`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
`72`		`-`
`73`	`71`	`/* Extract the I/O buffer chain from the container and free the`
`74`	`72`	`* container.`
`75`	`73`	`*/`
`76`	`74`
`77`	`75`	`iob = qentry->qe_head;`
`78`	`76`	`iob_free_qentry(qentry);`
`79`	`77`	`}`
`80`		`- else`
`81`		`- {`
`82`		`- spin_unlock_irqrestore(&g_iob_lock, flags);`
`83`		`- }`
`84`	`78`
	`79`	`+ leave_critical_section(flags);`
`85`	`80`	`return iob;`
`86`	`81`	`}`
`87`	`82`