diff --git a/mm/iob/Kconfig b/mm/iob/Kconfig index 3d603fde380..0790961a794 100644 --- a/mm/iob/Kconfig +++ b/mm/iob/Kconfig @@ -63,12 +63,15 @@ config IOB_THROTTLE config IOB_DEBUG bool "Force I/O buffer debug" default n - depends on DEBUG_FEATURES + depends on DEBUG_FEATURES && !SYSLOG_BUFFER ---help--- This option will force debug output from I/O buffer logic. This 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 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 endmenu # Common I/O buffer support diff --git a/mm/iob/iob.h b/mm/iob/iob.h index 5f4436980c1..9f43612d710 100644 --- a/mm/iob/iob.h +++ b/mm/iob/iob.h @@ -91,10 +91,18 @@ 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 +/* A list of all free, unallocated I/O buffer queue containers */ + 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 /* Counting semaphores that tracks the number of free IOBs/qentries */ diff --git a/mm/iob/iob_alloc.c b/mm/iob/iob_alloc.c index 27de2946706..9aee7ab8d43 100644 --- a/mm/iob/iob_alloc.c +++ b/mm/iob/iob_alloc.c @@ -1,7 +1,7 @@ /**************************************************************************** * 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 * * Redistribution and use in source and binary forms, with or without @@ -54,6 +54,47 @@ * 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 * @@ -85,73 +126,76 @@ static FAR struct iob_s *iob_allocwait(bool throttled) */ 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 - * will be decremented atomically. + /* If not successful, then the semaphore count was less than or equal + * 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); - if (!iob) + ret = sem_wait(sem); + if (ret < 0) { - /* If not successful, then the semaphore count was less than or - * 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 - * count will be incremented. + int errcode = get_errno(); + + /* 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 are in + * internal, non-user logic. But are there cases where the error + * should be returned. */ - ret = sem_wait(sem); - if (ret < 0) + if (errcode == EINTR) { - 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) - { - /* 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; - } + ret = 0; } else { - /* When we wake up from wait successfully, an I/O buffer was - * returned to the free list. However, if there are concurrent - * allocations from interrupt handling, then I suspect that - * there is a race condition. But no harm, we will just wait - * again in that case. + /* Stop the loop and return a error */ + + DEBUGASSERT(errcode > 0); + ret = -errcode; + } + } + 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 * iob_tryalloc(), perhaps allowing another thread to take our * count. In that event, iob_tryalloc() will fail above and * 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); + iob = iob_tryalloc(throttled); } } } - while (ret == OK && iob == NULL); leave_critical_section(flags); 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 */ iob = g_iob_freelist; - if (iob) + if (iob != NULL) { /* Remove the I/O buffer from the free list and decrement the * counting semaphore(s) that tracks the number of available diff --git a/mm/iob/iob_alloc_qentry.c b/mm/iob/iob_alloc_qentry.c index a1d4044bc45..2a54358b71f 100644 --- a/mm/iob/iob_alloc_qentry.c +++ b/mm/iob/iob_alloc_qentry.c @@ -55,6 +55,44 @@ * 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 * @@ -78,73 +116,78 @@ static FAR struct iob_qentry_s *iob_allocwait_qentry(void) */ 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 - * semaphore count will be decremented atomically. + /* If not successful, then the semaphore count was less than or equal + * 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(); - if (!qentry) + ret = sem_wait(&g_qentry_sem); + if (ret < 0) { - /* If not successful, then the semaphore count was less than or - * equal 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. + int errcode = get_errno(); + + /* 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. */ - ret = sem_wait(&g_qentry_sem); - if (ret < 0) + if (errcode == EINTR) { - int errcode = get_errno(); - - /* 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. + /* Force a success indication so that we will continue + * looping. */ - 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; - } + ret = 0; } else { - /* When we wake up from wait successfully, an I/O buffer chain - * container was returned to the free list. However, if there - * are concurrent allocations from interrupt handling, then I - * suspect that there is a race condition. But no harm, we - * will just wait again in that case. + /* Stop the loop and return a error */ + + DEBUGASSERT(errcode > 0); + ret = -errcode; + } + } + 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 - * iob_tryalloc_qentry(), perhaps allowing another thread to - * take our count. In that event, iob_tryalloc_qentry() will - * fail above and we will have to wait again. - * - * TODO: Consider a design modification to permit us to - * complete the allocation without losing our count. + * iob_tryalloc(), perhaps allowing another thread to take our + * count. In that event, iob_tryalloc() will fail above and + * we will have to wait again. */ sem_post(&g_qentry_sem); + qentry = iob_tryalloc_qentry(); } } } - while (ret == OK && !qentry); leave_critical_section(flags); return qentry; diff --git a/mm/iob/iob_free.c b/mm/iob/iob_free.c index 75a01cf89b6..0265e2d899a 100644 --- a/mm/iob/iob_free.c +++ b/mm/iob/iob_free.c @@ -74,7 +74,7 @@ FAR struct iob_s *iob_free(FAR struct iob_s *iob) * the next entry. */ - if (next) + if (next != NULL) { /* Copy and decrement the total packet length, being careful to * 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); } - /* Free the I/O buffer by adding it to the head of the free list. 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. + /* Free the I/O buffer by adding it to the head of the free or the + * committed list. 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 = 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); #if CONFIG_IOB_THROTTLE > 0 diff --git a/mm/iob/iob_free_qentry.c b/mm/iob/iob_free_qentry.c index 393f0ee47b6..135cd3ae3ae 100644 --- a/mm/iob/iob_free_qentry.c +++ b/mm/iob/iob_free_qentry.c @@ -1,7 +1,7 @@ /**************************************************************************** * 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 * * 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; irqstate_t flags; - /* Free the I/O buffer chain container by adding it to the head of the free - * list. 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. + /* Free the I/O buffer chain container by adding it to the head of the + * free or the committed list. 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 = 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); leave_critical_section(flags); diff --git a/mm/iob/iob_initialize.c b/mm/iob/iob_initialize.c index 1662fe47728..6c0df544549 100644 --- a/mm/iob/iob_initialize.c +++ b/mm/iob/iob_initialize.c @@ -1,7 +1,7 @@ /**************************************************************************** * 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 * * Redistribution and use in source and binary forms, with or without @@ -46,6 +46,14 @@ #include "iob.h" +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#ifndef NULL +# define NULL ((FAR void *)0) +#endif + /**************************************************************************** * Private Data ****************************************************************************/ @@ -65,10 +73,18 @@ static struct iob_qentry_s g_iob_qpool[CONFIG_IOB_NCHAINS]; 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 +/* A list of all free, unallocated I/O buffer queue containers */ + 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 /* Counting semaphores that tracks the number of free IOBs/qentries */ @@ -114,8 +130,9 @@ void iob_initialize(void) 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 sem_init(&g_throttle_sem, 0, CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE); #endif @@ -133,6 +150,8 @@ void iob_initialize(void) g_iob_freeqlist = iobq; } + g_iob_qcommitted = NULL; + sem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS); #endif initialized = true;