sergiotarxz/nuttx
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Networking: The are issues with the TCP write-ahead buffering if CONFIG_NET_NOINTS is enabled: There is a possibility of deadlocks in certain timing conditions. I have not seen this with the Tiva driver that I have been users but other people claim to see the issue on other platforms. Certainly it is a logic error: The network should never wait for TCP read-ahead buffering space to be available. It should drop the packets immediately.

Browse Source 
This was fixed by duplicating most of the IOB interfaces:  The versions that waited are still present (like iob_alloc()), but now there are non-waiting verisons of the same interfaces (like iob_tryalloc()).  The TCP read-ahead logic now uses only these non-waiting interfaces.
This commit is contained in:
Gregory Nutt 2015-01-27 21:23:42 -06:00
parent 80cfc0bdd3
commit c4bd6f52b5
7 changed files with 298 additions and 160 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
include/nuttx/net/iob.h
View File

@ -219,6 +219,19 @@ void iob_free_chain(FAR struct iob_s *iob);
int iob_add_queue(FAR struct iob_s *iob, FAR struct iob_queue_s *iobq);
#endif /* CONFIG_IOB_NCHAINS > 0 */
/****************************************************************************
* Name: iob_tryadd_queue
*
* Description:
* Add one I/O buffer chain to the end of a queue without waiting for
* resources to become free.
*
****************************************************************************/
#if CONFIG_IOB_NCHAINS > 0
int iob_tryadd_queue(FAR struct iob_s *iob, FAR struct iob_queue_s *iobq);
#endif /* CONFIG_IOB_NCHAINS > 0 */
/****************************************************************************
* Name: iob_remove_queue
*
@ -277,6 +290,19 @@ void iob_free_queue(FAR struct iob_queue_s *qhead);
int iob_copyin(FAR struct iob_s *iob, FAR const uint8_t *src,
unsigned int len, unsigned int offset, bool throttled);
/****************************************************************************
* Name: iob_trycopyin
*
* Description:
* Copy data 'len' bytes from a user buffer into the I/O buffer chain,
* starting at 'offset', extending the chain as necessary BUT without
* waiting if buffers are not available.
*
****************************************************************************/
int iob_trycopyin(FAR struct iob_s *iob, FAR const uint8_t *src,
unsigned int len, unsigned int offset, bool throttled);
/****************************************************************************
* Name: iob_copyout
*

23
net/iob/iob.h
View File

@ -98,6 +98,29 @@ extern sem_t g_qentry_sem; /* Counts free I/O buffer queue containers */
FAR struct iob_qentry_s *iob_alloc_qentry(void);
/****************************************************************************
* Name: iob_tryalloc_qentry
*
* Description:
* Try to allocate an I/O buffer chain container by taking the buffer at
* the head of the free list without waiting for the container to become
* free. This function is intended only for internal use by the IOB module.
*
****************************************************************************/
FAR struct iob_qentry_s *iob_tryalloc_qentry(void);
/****************************************************************************
* Name: iob_tryalloc
*
* Description:
* Try to allocate an I/O buffer by taking the buffer at the head of the
* free list without waiting for a buffer to become free.
*
****************************************************************************/
FAR struct iob_s *iob_tryalloc(bool throttled);
/****************************************************************************
* Name: iob_free_qentry
*

84
net/iob/iob_add_queue.c
View File

@ -64,6 +64,45 @@
# define NULL ((FAR void *)0)
#endif
/****************************************************************************
* Private Functions
****************************************************************************/
/****************************************************************************
* Name: iob_add_queue_internal
*
* Description:
* Add one I/O buffer chain to the end of a queue. May fail due to lack
* of resources.
*
****************************************************************************/
static int iob_add_queue_internal(FAR struct iob_s *iob,
FAR struct iob_queue_s *iobq,
FAR struct iob_qentry_s *qentry)
{
/* Add the I/O buffer chain to the container */
qentry->qe_head = iob;
/* Add the container to the end of the queue */
qentry->qe_flink = NULL;
if (!iobq->qh_head)
{
iobq->qh_head = qentry;
iobq->qh_tail = qentry;
}
else
{
DEBUGASSERT(iobq->qh_tail);
iobq->qh_tail->qe_flink = qentry;
iobq->qh_tail = qentry;
}
return 0;
}
/****************************************************************************
* Public Functions
****************************************************************************/
@ -90,26 +129,31 @@ int iob_add_queue(FAR struct iob_s *iob, FAR struct iob_queue_s *iobq)
return -ENOMEM;
}
/* Add the I/O buffer chain to the container */
qentry->qe_head = iob;
/* Add the container to the end of the queue */
qentry->qe_flink = NULL;
if (!iobq->qh_head)
{
iobq->qh_head = qentry;
iobq->qh_tail = qentry;
}
else
{
DEBUGASSERT(iobq->qh_tail);
iobq->qh_tail->qe_flink = qentry;
iobq->qh_tail = qentry;
}
return 0;
return iob_add_queue_internal(iob, iobq, qentry);
}
/****************************************************************************
* Name: iob_tryadd_queue
*
* Description:
* Add one I/O buffer chain to the end of a queue without waiting for
* resources to become free.
*
****************************************************************************/
int iob_tryadd_queue(FAR struct iob_s *iob, FAR struct iob_queue_s *iobq)
{
FAR struct iob_qentry_s *qentry;
/* Allocate a container to hold the I/O buffer chain */
qentry = iob_tryalloc_qentry();
if (!qentry)
{
ndbg("ERROR: Failed to allocate a container\n");
return -ENOMEM;
}
return iob_add_queue_internal(iob, iobq, qentry);
}
#endif /* CONFIG_IOB_NCHAINS > 0 */

162
net/iob/iob_alloc.c
View File

@ -74,87 +74,6 @@
* Private Functions
****************************************************************************/
/****************************************************************************
* Name: iob_tryalloc
*
* Description:
* Try to allocate an I/O buffer by taking the buffer at the head of the
* free list.
*
****************************************************************************/
static FAR struct iob_s *iob_tryalloc(bool throttled)
{
FAR struct iob_s *iob;
irqstate_t flags;
#if CONFIG_IOB_THROTTLE > 0
FAR sem_t *sem;
#endif
#if CONFIG_IOB_THROTTLE > 0
/* Select the semaphore count to check. */
sem = (throttled ? &g_throttle_sem : &g_iob_sem);
#endif
/* We don't know what context we are called from so we use extreme measures
* to protect the free list: We disable interrupts very briefly.
*/
flags = irqsave();
#if CONFIG_IOB_THROTTLE > 0
/* If there are free I/O buffers for this allocation */
if (sem->semcount > 0)
#endif
{
/* Take the I/O buffer from the head of the free list */
iob = g_iob_freelist;
if (iob)
{
/* Remove the I/O buffer from the free list and decrement the
* counting semaphore(s) that tracks the number of available
* IOBs.
*/
g_iob_freelist = iob->io_flink;
/* Take a semaphore count. Note that we cannot do this in
* in the orthodox way by calling sem_wait() or sem_trywait()
* because this function may be called from an interrupt
* handler. Fortunately we know at at least one free buffer
* so a simple decrement is all that is needed.
*/
g_iob_sem.semcount--;
DEBUGASSERT(g_iob_sem.semcount >= 0);
#if CONFIG_IOB_THROTTLE > 0
/* The throttle semaphore is a little more complicated because
* it can be negative! Decrementing is still safe, however.
*/
g_throttle_sem.semcount--;
DEBUGASSERT(g_throttle_sem.semcount >= -CONFIG_IOB_THROTTLE);
#endif
irqrestore(flags);
/* 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 */
return iob;
}
}
irqrestore(flags);
return NULL;
}
/****************************************************************************
* Name: iob_allocwait
*
@ -246,3 +165,84 @@ FAR struct iob_s *iob_alloc(bool throttled)
return iob_allocwait(throttled);
}
}
/****************************************************************************
* Name: iob_tryalloc
*
* Description:
* Try to allocate an I/O buffer by taking the buffer at the head of the
* free list without waiting for a buffer to become free.
*
****************************************************************************/
FAR struct iob_s *iob_tryalloc(bool throttled)
{
FAR struct iob_s *iob;
irqstate_t flags;
#if CONFIG_IOB_THROTTLE > 0
FAR sem_t *sem;
#endif
#if CONFIG_IOB_THROTTLE > 0
/* Select the semaphore count to check. */
sem = (throttled ? &g_throttle_sem : &g_iob_sem);
#endif
/* We don't know what context we are called from so we use extreme measures
* to protect the free list: We disable interrupts very briefly.
*/
flags = irqsave();
#if CONFIG_IOB_THROTTLE > 0
/* If there are free I/O buffers for this allocation */
if (sem->semcount > 0)
#endif
{
/* Take the I/O buffer from the head of the free list */
iob = g_iob_freelist;
if (iob)
{
/* Remove the I/O buffer from the free list and decrement the
* counting semaphore(s) that tracks the number of available
* IOBs.
*/
g_iob_freelist = iob->io_flink;
/* Take a semaphore count. Note that we cannot do this in
* in the orthodox way by calling sem_wait() or sem_trywait()
* because this function may be called from an interrupt
* handler. Fortunately we know at at least one free buffer
* so a simple decrement is all that is needed.
*/
g_iob_sem.semcount--;
DEBUGASSERT(g_iob_sem.semcount >= 0);
#if CONFIG_IOB_THROTTLE > 0
/* The throttle semaphore is a little more complicated because
* it can be negative! Decrementing is still safe, however.
*/
g_throttle_sem.semcount--;
DEBUGASSERT(g_throttle_sem.semcount >= -CONFIG_IOB_THROTTLE);
#endif
irqrestore(flags);
/* 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 */
return iob;
}
}
irqrestore(flags);
return NULL;
}

98
net/iob/iob_alloc_qentry.c
View File

@ -76,55 +76,6 @@
* Private Functions
****************************************************************************/
/****************************************************************************
* Name: iob_tryalloc_qentry
*
* Description:
* Try to allocate an I/O buffer chain container by taking the buffer at
* the head of the free list. This function is intended only for internal
* use by the IOB module.
*
****************************************************************************/
static FAR struct iob_qentry_s *iob_tryalloc_qentry(void)
{
FAR struct iob_qentry_s *iobq;
irqstate_t flags;
/* We don't know what context we are called from so we use extreme measures
* to protect the free list: We disable interrupts very briefly.
*/
flags = irqsave();
iobq = g_iob_freeqlist;
if (iobq)
{
/* Remove the I/O buffer chain container from the free list and
* decrement the counting semaphore that tracks the number of free
* containers.
*/
g_iob_freeqlist = iobq->qe_flink;
/* Take a semaphore count. Note that we cannot do this in
* in the orthodox way by calling sem_wait() or sem_trywait()
* because this function may be called from an interrupt
* handler. Fortunately we know at at least one free buffer
* so a simple decrement is all that is needed.
*/
g_qentry_sem.semcount--;
DEBUGASSERT(g_qentry_sem.semcount >= 0);
/* Put the I/O buffer in a known state */
iobq->qe_head = NULL; /* Nothing is contained */
}
irqrestore(flags);
return iobq;
}
/****************************************************************************
* Name: iob_allocwait_qentry
*
@ -211,4 +162,53 @@ FAR struct iob_qentry_s *iob_alloc_qentry(void)
}
}
/****************************************************************************
* Name: iob_tryalloc_qentry
*
* Description:
* Try to allocate an I/O buffer chain container by taking the buffer at
* the head of the free list without waiting for the container to become
* free. This function is intended only for internal use by the IOB module.
*
****************************************************************************/
FAR struct iob_qentry_s *iob_tryalloc_qentry(void)
{
FAR struct iob_qentry_s *iobq;
irqstate_t flags;
/* We don't know what context we are called from so we use extreme measures
* to protect the free list: We disable interrupts very briefly.
*/
flags = irqsave();
iobq = g_iob_freeqlist;
if (iobq)
{
/* Remove the I/O buffer chain container from the free list and
* decrement the counting semaphore that tracks the number of free
* containers.
*/
g_iob_freeqlist = iobq->qe_flink;
/* Take a semaphore count. Note that we cannot do this in
* in the orthodox way by calling sem_wait() or sem_trywait()
* because this function may be called from an interrupt
* handler. Fortunately we know at at least one free buffer
* so a simple decrement is all that is needed.
*/
g_qentry_sem.semcount--;
DEBUGASSERT(g_qentry_sem.semcount >= 0);
/* Put the I/O buffer in a known state */
iobq->qe_head = NULL; /* Nothing is contained */
}
irqrestore(flags);
return iobq;
}
#endif /* CONFIG_IOB_NCHAINS > 0 */

47
net/iob/iob_copyin.c
View File

@ -68,6 +68,8 @@
* Private Types
****************************************************************************/
typedef CODE struct iob_s *(*iob_alloc_t)(bool throttled);
/****************************************************************************
* Private Data
****************************************************************************/
@ -81,7 +83,7 @@
****************************************************************************/
/****************************************************************************
* Name: iob_copyin
* Name: iob_copyin_internal
*
* Description:
* Copy data 'len' bytes from a user buffer into the I/O buffer chain,
@ -89,8 +91,9 @@
*
****************************************************************************/
int iob_copyin(FAR struct iob_s *iob, FAR const uint8_t *src,
unsigned int len, unsigned int offset, bool throttled)
static int iob_copyin_internal(FAR struct iob_s *iob, FAR const uint8_t *src,
unsigned int len, unsigned int offset,
bool throttled, iob_alloc_t allocator)
{
FAR struct iob_s *head = iob;
FAR struct iob_s *next;
@ -206,7 +209,7 @@ int iob_copyin(FAR struct iob_s *iob, FAR const uint8_t *src,
{
/* Yes.. allocate a new buffer */
next = iob_alloc(throttled);
next = allocator(throttled);
if (next == NULL)
{
ndbg("ERROR: Failed to allocate I/O buffer\n");
@ -225,3 +228,39 @@ int iob_copyin(FAR struct iob_s *iob, FAR const uint8_t *src,
return OK;
}
/****************************************************************************
* Public Functions
****************************************************************************/
/****************************************************************************
* Name: iob_copyin
*
* Description:
* Copy data 'len' bytes from a user buffer into the I/O buffer chain,
* starting at 'offset', extending the chain as necessary.
*
****************************************************************************/
int iob_copyin(FAR struct iob_s *iob, FAR const uint8_t *src,
unsigned int len, unsigned int offset, bool throttled)
{
return iob_copyin_internal(iob, src, len, offset, throttled, iob_alloc);
}
/****************************************************************************
* Name: iob_trycopyin
*
* Description:
* Copy data 'len' bytes from a user buffer into the I/O buffer chain,
* starting at 'offset', extending the chain as necessary BUT without
* waiting if buffers are not available.
*
****************************************************************************/
int iob_trycopyin(FAR struct iob_s *iob, FAR const uint8_t *src,
unsigned int len, unsigned int offset, bool throttled)
{
return iob_copyin_internal(iob, src, len, offset, throttled, iob_tryalloc);
}

18
net/tcp/tcp_callback.c
View File

@ -50,6 +50,7 @@
#include <nuttx/net/netstats.h>
#include "devif/devif.h"
#include "iob/iob.h"
#include "tcp/tcp.h"
/****************************************************************************
@ -239,18 +240,21 @@ uint16_t tcp_datahandler(FAR struct tcp_conn_s *conn, FAR uint8_t *buffer,
FAR struct iob_s *iob;
int ret;
/* Allocate on I/O buffer to start the chain (throttling as necessary) */
/* Try to allocate on I/O buffer to start the chain without waiting (and
* throttling as necessary). If we would have to wait, then drop the
* packet.
*/
iob = iob_alloc(true);
iob = iob_tryalloc(true);
if (iob == NULL)
{
nlldbg("ERROR: Failed to create new I/O buffer chain\n");
return 0;
}
/* Copy the new appdata into the I/O buffer chain */
/* Copy the new appdata into the I/O buffer chain (without waiting) */
ret = iob_copyin(iob, buffer, buflen, 0, true);
ret = iob_trycopyin(iob, buffer, buflen, 0, true);
if (ret < 0)
{
/* On a failure, iob_copyin return a negated error value but does
@ -262,9 +266,11 @@ uint16_t tcp_datahandler(FAR struct tcp_conn_s *conn, FAR uint8_t *buffer,
return 0;
}
/* Add the new I/O buffer chain to the tail of the read-ahead queue */
/* Add the new I/O buffer chain to the tail of the read-ahead queue (again
* without waiting).
*/
ret = iob_add_queue(iob, &conn->readahead);
ret = iob_tryadd_queue(iob, &conn->readahead);
if (ret < 0)
{
nlldbg("ERROR: Failed to queue the I/O buffer chain: %d\n", ret);