Mirrors_Framework/nuttx
1
0
Fork 0
mirror of https://github.com/apache/nuttx.git synced 2026-06-04 06:42:32 +08:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

There can be a failure in IOB allocation to some asynchronous behavior caused by the use of sem_post(). Consider this scenario:

Browse Source 
Task A holds an IOB.  There are no further IOBs.  The value of semcount is zero.
Task B calls iob_alloc().  Since there are not IOBs, it calls sem_wait().  The v
alue of semcount is now -1.

Task A frees the IOB.  iob_free() adds the IOB to the free list and calls sem_post() this makes Task B ready to run and sets semcount to zero NOT 1.  There is one IOB in the free list and semcount is zero.  When Task B wakes up it would increment the sem_count back to the correct value.

But an interrupt or another task runs occurs before Task B executes.  The interrupt or other tak takes the IOB off of the free list and decrements the semcount.  But since semcount is then < 0, this causes the assertion because that is an invalid state in the interrupt handler.

So I think that the root cause is that there the asynchrony between incrementing the semcount.  This change separates the list of IOBs:  Currently there is only a free list of IOBs.  The problem, I believe, is because of asynchronies due sem_post() post cause the semcount and the list content to become out of sync.  This change adds a new 'committed' list:  When there is a task waiting for an IOB, it will go into the committed list rather than the free list before the semaphore is posted.  On the waiting side, when awakened from the semaphore wait, it will expect to find its IOB in the committed list, rather than free list.

In this way, the content of the free list and the value of the semaphore count always remain in sync.
This commit is contained in:
Gregory Nutt
2017-05-16 11:03:35 -06:00
parent a6e556d31c
commit 6a3800f611
7 changed files with 271 additions and 113 deletions
Download Patch File Download Diff File Expand all files Collapse all files
mm/iob/Kconfig
+4 -1
View File
@@ -63,12 +63,15 @@ config IOB_THROTTLE
config IOB_DEBUG config IOB_DEBUG
bool "Force I/O buffer debug" bool "Force I/O buffer debug"
default n default n
depends on DEBUG_FEATURES depends on DEBUG_FEATURES && !SYSLOG_BUFFER
---help--- ---help---
This option will force debug output from I/O buffer logic. This This option will force debug output from I/O buffer logic. This
is not normally something that would want to do but is convenient is not normally something that would want to do but is convenient
if you are debugging the I/O buffer logic and do not want to get if you are debugging the I/O buffer logic and do not want to get
overloaded with other un-related debug output. overloaded with other un-related debug output.
NOTE that this selection is not avaiable with IOBs are being used
to syslog buffering logic (CONFIG_SYSLOG_BUFFER=y)!
endif # MM_IOB endif # MM_IOB
endmenu # Common I/O buffer support endmenu # Common I/O buffer support
mm/iob/iob.h
+9 -1
View File
@@ -91,10 +91,18 @@
extern FAR struct iob_s *g_iob_freelist; extern FAR struct iob_s *g_iob_freelist;
/* A list of all free, unallocated I/O buffer queue containers */ /* A list of I/O buffers that are committed for allocation */
extern FAR struct iob_s *g_iob_committed;
#if CONFIG_IOB_NCHAINS > 0 #if CONFIG_IOB_NCHAINS > 0
/* A list of all free, unallocated I/O buffer queue containers */
extern FAR struct iob_qentry_s *g_iob_freeqlist; extern FAR struct iob_qentry_s *g_iob_freeqlist;
/* A list of I/O buffer queue containers that are committed for allocation */
extern FAR struct iob_s *g_iob_qcommitted;
#endif #endif
/* Counting semaphores that tracks the number of free IOBs/qentries */ /* Counting semaphores that tracks the number of free IOBs/qentries */
mm/iob/iob_alloc.c
+91 -47
View File
@@ -1,7 +1,7 @@
/**************************************************************************** /****************************************************************************
* mm/iob/iob_alloc.c * mm/iob/iob_alloc.c
* *
* Copyright (C) 2014, 2016 Gregory Nutt. All rights reserved. * Copyright (C) 2014, 2016-2017 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gnutt@nuttx.org> * Author: Gregory Nutt <gnutt@nuttx.org>
* *
* Redistribution and use in source and binary forms, with or without * Redistribution and use in source and binary forms, with or without
@@ -54,6 +54,47 @@
* Private Functions * Private Functions
****************************************************************************/ ****************************************************************************/
/****************************************************************************
* Name: iob_alloc_committed
*
* Description:
* Allocate an I/O buffer by taking the buffer at the head of the committed
* list.
*
****************************************************************************/
static FAR struct iob_s *iob_alloc_committed(void)
{
FAR struct iob_s *iob = NULL;
irqstate_t flags;
/* We don't know what context we are called from so we use extreme measures
* to protect the committed list: We disable interrupts very briefly.
*/
flags = enter_critical_section();
/* Take the I/O buffer from the head of the committed list */
iob = g_iob_committed;
if (iob != NULL)
{
/* Remove the I/O buffer from the committed list */
g_iob_committed = iob->io_flink;
/* Put the I/O buffer in a known state */
iob->io_flink = NULL; /* Not in a chain */
iob->io_len = 0; /* Length of the data in the entry */
iob->io_offset = 0; /* Offset to the beginning of data */
iob->io_pktlen = 0; /* Total length of the packet */
}
leave_critical_section(flags);
return iob;
}
/**************************************************************************** /****************************************************************************
* Name: iob_allocwait * Name: iob_allocwait
* *
@@ -85,73 +126,76 @@ static FAR struct iob_s *iob_allocwait(bool throttled)
*/ */
flags = enter_critical_section(); flags = enter_critical_section();
do
/* Try to get an I/O buffer. If successful, the semaphore count will be
* decremented atomically.
*/
iob = iob_tryalloc(throttled);
while (ret == OK && iob == NULL)
{ {
/* Try to get an I/O buffer. If successful, the semaphore count /* If not successful, then the semaphore count was less than or equal
* will be decremented atomically. * to zero (meaning that there are no free buffers). We need to wait
* for an I/O buffer to be released and placed in the committed
* list.
*/ */
iob = iob_tryalloc(throttled); ret = sem_wait(sem);
if (!iob) if (ret < 0)
{ {
/* If not successful, then the semaphore count was less than or int errcode = get_errno();
* equal to zero (meaning that there are no free buffers). We
* need to wait for an I/O buffer to be released when the semaphore /* EINTR is not an error! EINTR simply means that we were
* count will be incremented. * awakened by a signal and we should try again.
*
* REVISIT: Many end-user interfaces are required to return with
* an error if EINTR is set. Most uses of this function are in
* internal, non-user logic. But are there cases where the error
* should be returned.
*/ */
ret = sem_wait(sem); if (errcode == EINTR)
if (ret < 0)
{ {
int errcode = get_errno(); /* Force a success indication so that we will continue looping. */
/* EINTR is not an error! EINTR simply means that we were ret = 0;
* awakened by a signal and we should try again.
*
* REVISIT: Many end-user interfaces are required to return
* with an error if EINTR is set. Most uses of this function
* is in internal, non-user logic. But are there cases where
* the error should be returned.
*/
if (errcode == EINTR)
{
/* Force a success indication so that we will continue
* looping.
*/
ret = 0;
}
else
{
/* Stop the loop and return a error */
DEBUGASSERT(errcode > 0);
ret = -errcode;
}
} }
else else
{ {
/* When we wake up from wait successfully, an I/O buffer was /* Stop the loop and return a error */
* returned to the free list. However, if there are concurrent
* allocations from interrupt handling, then I suspect that DEBUGASSERT(errcode > 0);
* there is a race condition. But no harm, we will just wait ret = -errcode;
* again in that case. }
}
else
{
/* When we wake up from wait successfully, an I/O buffer was
* freed and we hold a count for one IOB. Unless somehting
* failed, we should have an IOB waiting for us in the
* committed list.
*/
iob = iob_alloc_committed();
DEBUGASSERT(iob != NULL);
if (iob == NULL)
{
/* This should not fail, but we allow for that possibility to
* handle any potential, non-obvious race condition. Perhaps
* the free IOB ended up in the g_iob_free list?
* *
* We need release our count so that it is available to * We need release our count so that it is available to
* iob_tryalloc(), perhaps allowing another thread to take our * iob_tryalloc(), perhaps allowing another thread to take our
* count. In that event, iob_tryalloc() will fail above and * count. In that event, iob_tryalloc() will fail above and
* we will have to wait again. * we will have to wait again.
*
* TODO: Consider a design modification to permit us to
* complete the allocation without losing our count.
*/ */
sem_post(sem); sem_post(sem);
iob = iob_tryalloc(throttled);
} }
} }
} }
while (ret == OK && iob == NULL);
leave_critical_section(flags); leave_critical_section(flags);
return iob; return iob;
@@ -225,7 +269,7 @@ FAR struct iob_s *iob_tryalloc(bool throttled)
/* Take the I/O buffer from the head of the free list */ /* Take the I/O buffer from the head of the free list */
iob = g_iob_freelist; iob = g_iob_freelist;
if (iob) if (iob != NULL)
{ {
/* Remove the I/O buffer from the free list and decrement the /* Remove the I/O buffer from the free list and decrement the
* counting semaphore(s) that tracks the number of available * counting semaphore(s) that tracks the number of available
mm/iob/iob_alloc_qentry.c
+90 -47
View File
@@ -55,6 +55,44 @@
* Private Functions * Private Functions
****************************************************************************/ ****************************************************************************/
/****************************************************************************
* Name: iob_alloc_qcommitted
*
* Description:
* Allocate an I/O buffer by taking the buffer at the head of the committed
* list.
*
****************************************************************************/
FAR struct iob_qentry_s *iob_alloc_qcommitted(void)
{
FAR struct iob_qentry_s *iobq = NULL;
irqstate_t flags;
/* We don't know what context we are called from so we use extreme measures
* to protect the committed list: We disable interrupts very briefly.
*/
flags = enter_critical_section();
/* Take the I/O buffer from the head of the committed list */
iobq = g_iob_qcommitted;
if (iobq != NULL)
{
/* Remove the I/O buffer from the committed list */
g_iob_qcommitted = iobq->io_flink;
/* Put the I/O buffer in a known state */
iobq->qe_head = NULL; /* Nothing is contained */
}
leave_critical_section(flags);
return iobq;
}
/**************************************************************************** /****************************************************************************
* Name: iob_allocwait_qentry * Name: iob_allocwait_qentry
* *
@@ -78,73 +116,78 @@ static FAR struct iob_qentry_s *iob_allocwait_qentry(void)
*/ */
flags = enter_critical_section(); flags = enter_critical_section();
do
/* Try to get an I/O buffer chain container. If successful, the semaphore
* count will bedecremented atomically.
*/
qentry = iob_tryalloc_qentry();
while (ret == OK && qentry == NULL)
{ {
/* Try to get an I/O buffer chain container. If successful, the /* If not successful, then the semaphore count was less than or equal
* semaphore count will be decremented atomically. * to zero (meaning that there are no free buffers). We need to wait
* for an I/O buffer chain container to be released when the
* semaphore count will be incremented.
*/ */
qentry = iob_tryalloc_qentry(); ret = sem_wait(&g_qentry_sem);
if (!qentry) if (ret < 0)
{ {
/* If not successful, then the semaphore count was less than or int errcode = get_errno();
* equal to zero (meaning that there are no free buffers). We
* need to wait for an I/O buffer chain container to be released /* EINTR is not an error! EINTR simply means that we were
* when the semaphore count will be incremented. * awakened by a signal and we should try again.
*
* REVISIT: Many end-user interfaces are required to return
* with an error if EINTR is set. Most uses of this function
* is in internal, non-user logic. But are there cases where
* the error should be returned.
*/ */
ret = sem_wait(&g_qentry_sem); if (errcode == EINTR)
if (ret < 0)
{ {
int errcode = get_errno(); /* Force a success indication so that we will continue
* looping.
/* EINTR is not an error! EINTR simply means that we were
* awakened by a signal and we should try again.
*
* REVISIT: Many end-user interfaces are required to return
* with an error if EINTR is set. Most uses of this function
* is in internal, non-user logic. But are there cases where
* the error should be returned.
*/ */
if (errcode == EINTR) ret = 0;
{
/* Force a success indication so that we will continue
* looping.
*/
ret = 0;
}
else
{
/* Stop the loop and return a error */
DEBUGASSERT(errcode > 0);
ret = -errcode;
}
} }
else else
{ {
/* When we wake up from wait successfully, an I/O buffer chain /* Stop the loop and return a error */
* container was returned to the free list. However, if there
* are concurrent allocations from interrupt handling, then I DEBUGASSERT(errcode > 0);
* suspect that there is a race condition. But no harm, we ret = -errcode;
* will just wait again in that case. }
}
else
{
/* When we wake up from wait successfully, an I/O buffer chain container was
* freed and we hold a count for one IOB. Unless somehting
* failed, we should have an IOB waiting for us in the
* committed list.
*/
qentry = iob_alloc_qcommitted();
DEBUGASSERT(qentry != NULL);
if (qentry == NULL)
{
/* This should not fail, but we allow for that possibility to
* handle any potential, non-obvious race condition. Perhaps
* the free IOB ended up in the g_iob_free list?
* *
* We need release our count so that it is available to * We need release our count so that it is available to
* iob_tryalloc_qentry(), perhaps allowing another thread to * iob_tryalloc(), perhaps allowing another thread to take our
* take our count. In that event, iob_tryalloc_qentry() will * count. In that event, iob_tryalloc() will fail above and
* fail above and we will have to wait again. * we will have to wait again.
*
* TODO: Consider a design modification to permit us to
* complete the allocation without losing our count.
*/ */
sem_post(&g_qentry_sem); sem_post(&g_qentry_sem);
qentry = iob_tryalloc_qentry();
} }
} }
} }
while (ret == OK && !qentry);
leave_critical_section(flags); leave_critical_section(flags);
return qentry; return qentry;
mm/iob/iob_free.c
+27 -7
View File
@@ -74,7 +74,7 @@ FAR struct iob_s *iob_free(FAR struct iob_s *iob)
* the next entry. * the next entry.
*/ */
if (next) if (next != NULL)
{ {
/* Copy and decrement the total packet length, being careful to /* Copy and decrement the total packet length, being careful to
* do nothing too crazy. * do nothing too crazy.
@@ -101,16 +101,36 @@ FAR struct iob_s *iob_free(FAR struct iob_s *iob)
next, next->io_pktlen, next->io_len); next, next->io_pktlen, next->io_len);
} }
/* Free the I/O buffer by adding it to the head of the free list. We don't /* Free the I/O buffer by adding it to the head of the free or the
* know what context we are called from so we use extreme measures to * committed list. We don't know what context we are called from so
* protect the free list: We disable interrupts very briefly. * we use extreme measures to protect the free list: We disable
* interrupts very briefly.
*/ */
flags = enter_critical_section(); flags = enter_critical_section();
iob->io_flink = g_iob_freelist;
g_iob_freelist = iob;
/* Signal that an IOB is available */ /* 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
* it is reserved for that allocation (and not available to
* iob_tryalloc()).
*/
if (g_iob_sem.semcount < 0)
{
iob->io_flink = g_iob_committed;
g_iob_committed = iob;
}
else
{
iob->io_flink = g_iob_freelist;
g_iob_freelist = iob;
}
/* Signal that an IOB is available. If there is a thread waiting
* for an IOB, this will wake up exactly one thread. The semaphore
* count will correctly indicated that the awakened task owns an
* IOB and should find it in the committed list.
*/
sem_post(&g_iob_sem); sem_post(&g_iob_sem);
#if CONFIG_IOB_THROTTLE > 0 #if CONFIG_IOB_THROTTLE > 0
mm/iob/iob_free_qentry.c
+28 -7
View File
@@ -1,7 +1,7 @@
/**************************************************************************** /****************************************************************************
* mm/iob/iob_free_qentry.c * mm/iob/iob_free_qentry.c
* *
* Copyright (C) 2014, 2016 Gregory Nutt. All rights reserved. * Copyright (C) 2014, 2016-2017 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gnutt@nuttx.org> * Author: Gregory Nutt <gnutt@nuttx.org>
* *
* Redistribution and use in source and binary forms, with or without * Redistribution and use in source and binary forms, with or without
@@ -68,16 +68,37 @@ FAR struct iob_qentry_s *iob_free_qentry(FAR struct iob_qentry_s *iobq)
FAR struct iob_qentry_s *nextq = iobq->qe_flink; FAR struct iob_qentry_s *nextq = iobq->qe_flink;
irqstate_t flags; irqstate_t flags;
/* Free the I/O buffer chain container by adding it to the head of the free /* Free the I/O buffer chain container by adding it to the head of the
* list. We don't know what context we are called from so we use extreme * free or the committed list. We don't know what context we are called
* measures to protect the free list: We disable interrupts very briefly. * from so we use extreme measures to protect the free list: We disable
* interrupts very briefly.
*/ */
flags = enter_critical_section(); flags = enter_critical_section();
iobq->qe_flink = g_iob_freeqlist;
g_iob_freeqlist = iobq;
/* Signal that an I/O buffer chain container is available */ /* 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
* it is reserved for that allocation (and not available to
* iob_tryalloc()).
*/
if (g_iob_sem.semcount < 0)
{
iobq->qe_flink = g_iob_qcommitted;
g_iob_qcommitted = iobq;
}
else
{
iobq->qe_flink = g_iob_freeqlist;
g_iob_freeqlist = iobq;
}
/* 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
* indicated that the awakened task owns an I/O buffer chain container
* and should find it in the committed list.
*/
sem_post(&g_qentry_sem); sem_post(&g_qentry_sem);
leave_critical_section(flags); leave_critical_section(flags);
mm/iob/iob_initialize.c
+22 -3
View File
@@ -1,7 +1,7 @@
/**************************************************************************** /****************************************************************************
* mm/iob/iob_initialize.c * mm/iob/iob_initialize.c
* *
* Copyright (C) 2014 Gregory Nutt. All rights reserved. * Copyright (C) 2014, 2017 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <gnutt@nuttx.org> * Author: Gregory Nutt <gnutt@nuttx.org>
* *
* Redistribution and use in source and binary forms, with or without * Redistribution and use in source and binary forms, with or without
@@ -46,6 +46,14 @@
#include "iob.h" #include "iob.h"
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
#ifndef NULL
# define NULL ((FAR void *)0)
#endif
/**************************************************************************** /****************************************************************************
* Private Data * Private Data
****************************************************************************/ ****************************************************************************/
@@ -65,10 +73,18 @@ static struct iob_qentry_s g_iob_qpool[CONFIG_IOB_NCHAINS];
FAR struct iob_s *g_iob_freelist; FAR struct iob_s *g_iob_freelist;
/* A list of all free, unallocated I/O buffer queue containers */ /* A list of I/O buffers that are committed for allocation */
FAR struct iob_s *g_iob_committed;
#if CONFIG_IOB_NCHAINS > 0 #if CONFIG_IOB_NCHAINS > 0
/* A list of all free, unallocated I/O buffer queue containers */
FAR struct iob_qentry_s *g_iob_freeqlist; FAR struct iob_qentry_s *g_iob_freeqlist;
/* A list of I/O buffer queue containers that are committed for allocation */
extern FAR struct iob_s *g_iob_qcommitted;
#endif #endif
/* Counting semaphores that tracks the number of free IOBs/qentries */ /* Counting semaphores that tracks the number of free IOBs/qentries */
@@ -114,8 +130,9 @@ void iob_initialize(void)
g_iob_freelist = iob; g_iob_freelist = iob;
} }
sem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS); g_iob_committed = NULL;
sem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
#if CONFIG_IOB_THROTTLE > 0 #if CONFIG_IOB_THROTTLE > 0
sem_init(&g_throttle_sem, 0, CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE); sem_init(&g_throttle_sem, 0, CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
#endif #endif
@@ -133,6 +150,8 @@ void iob_initialize(void)
g_iob_freeqlist = iobq; g_iob_freeqlist = iobq;
} }
g_iob_qcommitted = NULL;
sem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS); sem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
#endif #endif
initialized = true; initialized = true;