You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by gu...@apache.org on 2021/07/10 19:10:37 UTC
[incubator-nuttx] branch master updated (92210cb -> 5fe51b9)
This is an automated email from the ASF dual-hosted git repository.
gustavonihei pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git.
from 92210cb arch/sim: enable nxlooper
new f5279f8 sim: fix loop add delaylist when mm_free in IDLE thread
new 5fe51b9 mm: Simplify the semaphore handling
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
arch/sim/src/sim/up_heap.c | 21 +++--
mm/mm_heap/mm.h | 5 +-
mm/mm_heap/mm_checkcorruption.c | 13 +--
mm/mm_heap/mm_extend.c | 2 +-
mm/mm_heap/mm_free.c | 31 +------
mm/mm_heap/mm_initialize.c | 2 +-
mm/mm_heap/mm_mallinfo.c | 2 +-
mm/mm_heap/mm_malloc.c | 4 +-
mm/mm_heap/mm_memalign.c | 2 +-
mm/mm_heap/mm_realloc.c | 2 +-
mm/mm_heap/mm_sem.c | 192 ++++++++--------------------------------
11 files changed, 61 insertions(+), 215 deletions(-)
[incubator-nuttx] 02/02: mm: Simplify the semaphore handling
Posted by gu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
gustavonihei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit 5fe51b923a1021757a0a218b57d8e6d0c65ebf47
Author: Xiang Xiao <xi...@xiaomi.com>
AuthorDate: Wed Jul 7 21:21:42 2021 +0800
mm: Simplify the semaphore handling
1.Move all special process to mm_takesemaphore
2.Remove the support of recurive lock
3.Remove mm_trysemaphore function
Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
Change-Id: Ie216a6294ab67c5d427f31b089beb15c532f08fe
---
arch/sim/src/sim/up_heap.c | 23 +++--
mm/mm_heap/mm.h | 5 +-
mm/mm_heap/mm_checkcorruption.c | 13 +--
mm/mm_heap/mm_extend.c | 2 +-
mm/mm_heap/mm_free.c | 31 +------
mm/mm_heap/mm_initialize.c | 2 +-
mm/mm_heap/mm_mallinfo.c | 2 +-
mm/mm_heap/mm_malloc.c | 4 +-
mm/mm_heap/mm_memalign.c | 2 +-
mm/mm_heap/mm_realloc.c | 2 +-
mm/mm_heap/mm_sem.c | 192 ++++++++--------------------------------
11 files changed, 67 insertions(+), 211 deletions(-)
diff --git a/arch/sim/src/sim/up_heap.c b/arch/sim/src/sim/up_heap.c
index 615eb97..84b881c 100644
--- a/arch/sim/src/sim/up_heap.c
+++ b/arch/sim/src/sim/up_heap.c
@@ -65,9 +65,9 @@ struct mm_heap_s
* Private Functions
****************************************************************************/
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
static void mm_add_delaylist(FAR struct mm_heap_s *heap, FAR void *mem)
{
+#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
FAR struct mm_delaynode_s *tmp = mem;
irqstate_t flags;
@@ -79,9 +79,8 @@ static void mm_add_delaylist(FAR struct mm_heap_s *heap, FAR void *mem)
heap->mm_delaylist[up_cpu_index()] = tmp;
leave_critical_section(flags);
-}
-
#endif
+}
static void mm_free_delaylist(FAR struct mm_heap_s *heap)
{
@@ -212,17 +211,27 @@ FAR void *mm_malloc(FAR struct mm_heap_s *heap, size_t size)
FAR void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
{
#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
- if (getpid() == -ESRCH)
+ /* Check current environment */
+
+ if (up_interrupt_context())
+ {
+ /* We are in ISR, add to the delay list */
+
+ mm_add_delaylist(heap, mem);
+ }
+ else
+#endif
+
+ if (getpid() < 0)
{
/* getpid() return -ESRCH, means we are in situations
- * during context switching(See mm_trysemaphore() & getpid()).
- * Then add to mm_delaylist.
+ * during context switching(See getpid's comment).
+ * Then add to the delay list.
*/
mm_add_delaylist(heap, mem);
}
else
-#endif
{
host_free(mem);
}
diff --git a/mm/mm_heap/mm.h b/mm/mm_heap/mm.h
index 334c136..1565116 100644
--- a/mm/mm_heap/mm.h
+++ b/mm/mm_heap/mm.h
@@ -177,8 +177,6 @@ struct mm_heap_s
*/
sem_t mm_semaphore;
- pid_t mm_holder;
- int mm_counts_held;
/* This is the size of the heap provided to mm */
@@ -216,8 +214,7 @@ struct mm_heap_s
/* Functions contained in mm_sem.c ******************************************/
void mm_seminitialize(FAR struct mm_heap_s *heap);
-void mm_takesemaphore(FAR struct mm_heap_s *heap);
-int mm_trysemaphore(FAR struct mm_heap_s *heap);
+bool mm_takesemaphore(FAR struct mm_heap_s *heap);
void mm_givesemaphore(FAR struct mm_heap_s *heap);
/* Functions contained in mm_shrinkchunk.c **********************************/
diff --git a/mm/mm_heap/mm_checkcorruption.c b/mm/mm_heap/mm_checkcorruption.c
index 2790ca7..80412db 100644
--- a/mm/mm_heap/mm_checkcorruption.c
+++ b/mm/mm_heap/mm_checkcorruption.c
@@ -68,21 +68,10 @@ void mm_checkcorruption(FAR struct mm_heap_s *heap)
* Retake the semaphore for each region to reduce latencies
*/
- if (up_interrupt_context())
+ if (mm_takesemaphore(heap) == false)
{
return;
}
- else if (sched_idletask())
- {
- if (mm_trysemaphore(heap))
- {
- return;
- }
- }
- else
- {
- mm_takesemaphore(heap);
- }
for (node = heap->mm_heapstart[region];
node < heap->mm_heapend[region];
diff --git a/mm/mm_heap/mm_extend.c b/mm/mm_heap/mm_extend.c
index b1bcd2c..17fb0f0 100644
--- a/mm/mm_heap/mm_extend.c
+++ b/mm/mm_heap/mm_extend.c
@@ -77,7 +77,7 @@ void mm_extend(FAR struct mm_heap_s *heap, FAR void *mem, size_t size,
/* Take the memory manager semaphore */
- mm_takesemaphore(heap);
+ DEBUGVERIFY(mm_takesemaphore(heap));
/* Get the terminal node in the old heap. The block to extend must
* immediately follow this node.
diff --git a/mm/mm_heap/mm_free.c b/mm/mm_heap/mm_free.c
index 8ea5d80..c3545cf 100644
--- a/mm/mm_heap/mm_free.c
+++ b/mm/mm_heap/mm_free.c
@@ -36,9 +36,9 @@
* Private Functions
****************************************************************************/
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
static void mm_add_delaylist(FAR struct mm_heap_s *heap, FAR void *mem)
{
+#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
FAR struct mm_delaynode_s *tmp = mem;
irqstate_t flags;
@@ -50,8 +50,8 @@ static void mm_add_delaylist(FAR struct mm_heap_s *heap, FAR void *mem)
heap->mm_delaylist[up_cpu_index()] = tmp;
leave_critical_section(flags);
-}
#endif
+}
/****************************************************************************
* Public Functions
@@ -83,39 +83,16 @@ void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
return;
}
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
- /* Check current environment */
-
- if (up_interrupt_context())
- {
- /* We are in ISR, add to mm_delaylist */
-
- mm_add_delaylist(heap, mem);
- return;
- }
- else if ((ret = mm_trysemaphore(heap)) == 0)
- {
- /* Got the sem, do free immediately */
- }
- else if (ret == -ESRCH || sched_idletask())
+ if (mm_takesemaphore(heap) == false)
{
/* We are in IDLE task & can't get sem, or meet -ESRCH return,
* which means we are in situations during context switching(See
- * mm_trysemaphore() & getpid()). Then add to mm_delaylist.
+ * mm_takesemaphore() & getpid()). Then add to the delay list.
*/
mm_add_delaylist(heap, mem);
return;
}
- else
-#endif
- {
- /* We need to hold the MM semaphore while we muck with the
- * nodelist.
- */
-
- mm_takesemaphore(heap);
- }
DEBUGASSERT(mm_heapmember(heap, mem));
diff --git a/mm/mm_heap/mm_initialize.c b/mm/mm_heap/mm_initialize.c
index f73f7ae..cf6f207 100644
--- a/mm/mm_heap/mm_initialize.c
+++ b/mm/mm_heap/mm_initialize.c
@@ -86,7 +86,7 @@ void mm_addregion(FAR struct mm_heap_s *heap, FAR void *heapstart,
DEBUGASSERT(heapsize <= MMSIZE_MAX + 1);
#endif
- mm_takesemaphore(heap);
+ DEBUGVERIFY(mm_takesemaphore(heap));
/* Adjust the provide heap start and size so that they are both aligned
* with the MM_MIN_CHUNK size.
diff --git a/mm/mm_heap/mm_mallinfo.c b/mm/mm_heap/mm_mallinfo.c
index 657649e..b9a26af 100644
--- a/mm/mm_heap/mm_mallinfo.c
+++ b/mm/mm_heap/mm_mallinfo.c
@@ -73,7 +73,7 @@ int mm_mallinfo(FAR struct mm_heap_s *heap, FAR struct mallinfo *info)
* Retake the semaphore for each region to reduce latencies
*/
- mm_takesemaphore(heap);
+ DEBUGVERIFY(mm_takesemaphore(heap));
for (node = heap->mm_heapstart[region];
node < heap->mm_heapend[region];
diff --git a/mm/mm_heap/mm_malloc.c b/mm/mm_heap/mm_malloc.c
index 6cbd8e6..3695d0b 100644
--- a/mm/mm_heap/mm_malloc.c
+++ b/mm/mm_heap/mm_malloc.c
@@ -102,7 +102,7 @@ FAR void *mm_malloc(FAR struct mm_heap_s *heap, size_t size)
FAR void *ret = NULL;
int ndx;
- /* Firstly, free mm_delaylist */
+ /* Free the delay list first */
mm_free_delaylist(heap);
@@ -130,7 +130,7 @@ FAR void *mm_malloc(FAR struct mm_heap_s *heap, size_t size)
/* We need to hold the MM semaphore while we muck with the nodelist. */
- mm_takesemaphore(heap);
+ DEBUGVERIFY(mm_takesemaphore(heap));
/* Get the location in the node list to start the search. Special case
* really big allocations
diff --git a/mm/mm_heap/mm_memalign.c b/mm/mm_heap/mm_memalign.c
index fab4b97..9d1bdd6 100644
--- a/mm/mm_heap/mm_memalign.c
+++ b/mm/mm_heap/mm_memalign.c
@@ -115,7 +115,7 @@ FAR void *mm_memalign(FAR struct mm_heap_s *heap, size_t alignment,
* nodelist.
*/
- mm_takesemaphore(heap);
+ DEBUGVERIFY(mm_takesemaphore(heap));
/* Get the node associated with the allocation and the next node after
* the allocation.
diff --git a/mm/mm_heap/mm_realloc.c b/mm/mm_heap/mm_realloc.c
index 0e9fa11..093446e 100644
--- a/mm/mm_heap/mm_realloc.c
+++ b/mm/mm_heap/mm_realloc.c
@@ -107,7 +107,7 @@ FAR void *mm_realloc(FAR struct mm_heap_s *heap, FAR void *oldmem,
/* We need to hold the MM semaphore while we muck with the nodelist. */
- mm_takesemaphore(heap);
+ DEBUGVERIFY(mm_takesemaphore(heap));
DEBUGASSERT(oldnode->preceding & MM_ALLOC_BIT);
DEBUGASSERT(mm_heapmember(heap, oldmem));
diff --git a/mm/mm_heap/mm_sem.c b/mm/mm_heap/mm_sem.c
index 3bbc580..a4d4e86 100644
--- a/mm/mm_heap/mm_sem.c
+++ b/mm/mm_heap/mm_sem.c
@@ -29,36 +29,13 @@
#include <assert.h>
#include <debug.h>
+#include <nuttx/arch.h>
#include <nuttx/semaphore.h>
#include <nuttx/mm/mm.h>
#include "mm_heap/mm.h"
/****************************************************************************
- * Pre-processor Definitions
- ****************************************************************************/
-
-/* This is a special value that indicates that there is no holder of the
- * semaphore. The valid range of PIDs is 0-32767 and any value outside of
- * that range could be used (except -ESRCH which is a special return value
- * from getpid())
- */
-
-#define NO_HOLDER ((pid_t)-1)
-
-/* Define MONITOR_MM_SEMAPHORE to enable semaphore state monitoring */
-
-#ifdef MONITOR_MM_SEMAPHORE
-# define msemerr _err
-# define msemwarn _warn
-# define mseminfo _info
-#else
-# define msemerr _none
-# define msemwarn _none
-# define mseminfo _none
-#endif
-
-/****************************************************************************
* Public Functions
****************************************************************************/
@@ -77,127 +54,67 @@ void mm_seminitialize(FAR struct mm_heap_s *heap)
*/
_SEM_INIT(&heap->mm_semaphore, 0, 1);
-
- heap->mm_holder = NO_HOLDER;
- heap->mm_counts_held = 0;
}
/****************************************************************************
- * Name: mm_trysemaphore
+ * Name: mm_takesemaphore
*
* Description:
- * Try to take the MM mutex. This is called only from the OS in certain
- * conditions when it is necessary to have exclusive access to the memory
- * manager but it is impossible to wait on a semaphore (e.g., the idle
- * process when it performs its background memory cleanup).
+ * Take the MM mutex. This may be called from the OS in certain conditions
+ * when it is impossible to wait on a semaphore:
+ * 1.The idle process performs the memory corruption check.
+ * 2.The task/thread free the memory in the exiting process.
+ *
+ * Input Parameters:
+ * heap - heap instance want to take semaphore
+ *
+ * Returned Value:
+ * true if the semaphore can be taken, otherwise false.
*
****************************************************************************/
-int mm_trysemaphore(FAR struct mm_heap_s *heap)
+bool mm_takesemaphore(FAR struct mm_heap_s *heap)
{
- pid_t my_pid = getpid();
- int ret;
+#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+ /* Check current environment */
- /* getpid() returns the task ID of the task at the head of the ready-to-
- * run task list. mm_trysemaphore() may be called during context
- * switches. There are certain situations during context switching when
- * the OS data structures are in flux and where the current task (i.e.,
- * the task at the head of the ready-to-run task list) is not actually
- * running. Granting the semaphore access in that case is known to result
- * in heap corruption as in the following failure scenario.
- *
- * ---------------------------- -------------------------------
- * TASK A TASK B
- * ---------------------------- -------------------------------
- * Begins memory allocation.
- * - Holder is set to TASK B
- * <---- Task B preempted, Task A runs
- * Task A exits
- * - Current task set to Task B
- * Free tcb and stack memory
- * - Since holder is Task B,
- * memory manager is re-
- * entered, and
- * - Heap is corrupted.
- * ---------------------------- -------------------------------
- *
- * This is handled by getpid(): If the case where Task B is not actually
- * running, then getpid() will return the special value -ESRCH. That will
- * avoid taking the fatal 'if' logic and will fall through to use the
- * 'else', albeit with a nonsensical PID value.
- */
-
- if (my_pid < 0)
+ if (up_interrupt_context())
{
- ret = my_pid;
- goto errout;
+ /* Can't take semaphore in the interrupt handler */
+
+ return false;
}
+ else
+#endif
- /* Does the current task already hold the semaphore? Is the current
- * task actually running?
+ /* getpid() returns the task ID of the task at the head of the ready-to-
+ * run task list. mm_takesemaphore() may be called during context
+ * switches. There are certain situations during context switching when
+ * the OS data structures are in flux and then can't be freed immediately
+ * (e.g. the running thread stack).
+ *
+ * This is handled by getpid() to return the special value -ESRCH to
+ * indicate this special situation.
*/
- if (heap->mm_holder == my_pid)
+ if (getpid() < 0)
{
- /* Yes, just increment the number of references held by the current
- * task.
- */
-
- heap->mm_counts_held++;
- ret = OK;
+ return false;
}
- else
+#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+ else if (sched_idletask())
{
/* Try to take the semaphore */
- ret = _SEM_TRYWAIT(&heap->mm_semaphore);
- if (ret < 0)
- {
- ret = _SEM_ERRVAL(ret);
- goto errout;
- }
-
- /* We have it. Claim the heap for the current task and return */
-
- heap->mm_holder = my_pid;
- heap->mm_counts_held = 1;
- ret = OK;
- }
-
-errout:
- return ret;
-}
-
-/****************************************************************************
- * Name: mm_takesemaphore
- *
- * Description:
- * Take the MM mutex. This is the normal action before all memory
- * management actions.
- *
- ****************************************************************************/
-
-void mm_takesemaphore(FAR struct mm_heap_s *heap)
-{
- pid_t my_pid = getpid();
-
- /* Does the current task already hold the semaphore? */
-
- if (heap->mm_holder == my_pid)
- {
- /* Yes, just increment the number of references held by the current
- * task.
- */
-
- heap->mm_counts_held++;
+ return _SEM_TRYWAIT(&heap->mm_semaphore) >= 0;
}
+#endif
else
{
int ret;
/* Take the semaphore (perhaps waiting) */
- mseminfo("PID=%d taking\n", my_pid);
do
{
ret = _SEM_WAIT(&heap->mm_semaphore);
@@ -212,18 +129,10 @@ void mm_takesemaphore(FAR struct mm_heap_s *heap)
DEBUGASSERT(ret == -EINTR || ret == -ECANCELED);
}
}
- while (ret == -EINTR);
-
- /* We have it (or some awful, unexpected error occurred). Claim
- * the semaphore for the current task and return.
- */
+ while (ret < 0);
- heap->mm_holder = my_pid;
- heap->mm_counts_held = 1;
+ return true;
}
-
- mseminfo("Holder=%d count=%d\n", heap->mm_holder,
- heap->mm_counts_held);
}
/****************************************************************************
@@ -236,30 +145,5 @@ void mm_takesemaphore(FAR struct mm_heap_s *heap)
void mm_givesemaphore(FAR struct mm_heap_s *heap)
{
- /* The current task should be holding at least one reference to the
- * semaphore.
- */
-
- DEBUGASSERT(heap->mm_holder == getpid());
-
- /* Does the current task hold multiple references to the semaphore */
-
- if (heap->mm_counts_held > 1)
- {
- /* Yes, just release one count and return */
-
- heap->mm_counts_held--;
- mseminfo("Holder=%d count=%d\n", heap->mm_holder,
- heap->mm_counts_held);
- }
- else
- {
- /* Nope, this is the last reference held by the current task. */
-
- mseminfo("PID=%d giving\n", getpid());
-
- heap->mm_holder = NO_HOLDER;
- heap->mm_counts_held = 0;
- DEBUGVERIFY(_SEM_POST(&heap->mm_semaphore));
- }
+ DEBUGVERIFY(_SEM_POST(&heap->mm_semaphore));
}
[incubator-nuttx] 01/02: sim: fix loop add delaylist when mm_free
in IDLE thread
Posted by gu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
gustavonihei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit f5279f85839c0b061186cb677a7ae658f4c0adbf
Author: ligd <li...@xiaomi.com>
AuthorDate: Fri Jul 2 15:22:54 2021 +0800
sim: fix loop add delaylist when mm_free in IDLE thread
Change-Id: I1827c663275f47c9dc30d63e17e3d016b0000166
Signed-off-by: ligd <li...@xiaomi.com>
---
arch/sim/src/sim/up_heap.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/arch/sim/src/sim/up_heap.c b/arch/sim/src/sim/up_heap.c
index 22d6c10..615eb97 100644
--- a/arch/sim/src/sim/up_heap.c
+++ b/arch/sim/src/sim/up_heap.c
@@ -212,21 +212,11 @@ FAR void *mm_malloc(FAR struct mm_heap_s *heap, size_t size)
FAR void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
{
#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
- int ret = (int)getpid();
-
- /* Check current environment */
-
- if (up_interrupt_context())
- {
- /* We are in ISR, add to mm_delaylist */
-
- mm_add_delaylist(heap, mem);
- }
- else if (ret == -ESRCH || sched_idletask())
+ if (getpid() == -ESRCH)
{
- /* We are in IDLE task & can't get sem, or meet -ESRCH return,
- * which means we are in situations during context switching(See
- * mm_trysemaphore() & getpid()). Then add to mm_delaylist.
+ /* getpid() return -ESRCH, means we are in situations
+ * during context switching(See mm_trysemaphore() & getpid()).
+ * Then add to mm_delaylist.
*/
mm_add_delaylist(heap, mem);