You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/08 17:31:46 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #4096: mm: Simplify the semaphore handling
xiaoxiang781216 opened a new pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096
## Summary
1.Move all special process to mm_takesemaphore
2.Remove the support of recurive lock
3.Remove mm_trysemaphore function
## Impact
No, internal implementation change only.
## Testing
ostest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666627110
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
The memory will be freed in the next allocation(e.g. malloc/realloc/zalloc/memalign...).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666627882
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,33 @@ 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:
+ * return true if the semaphore can be taked, otherwise false.
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667111217
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
If "free should never be called in the interrupt context, so I add DEBUGASSERT(up_interrupt_context() == false)" is a true statement then explain why the [original code]( https://github.com/apache/incubator-nuttx/commit/1d2396353e5775db50794c59201731baf2dcc153 ) tested for it an used the delay queue.
The PR breaks that behavior. I am asking you to verify if that was a bad change or this one is.
If we supporting free from up_interrupt_context (like post) this will be a breaking change to anyone that used it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666453232
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
@xiaoxiang781216 Where is the equivalent test for the up_interrupt_context() to do a delayed action?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667171760
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
Did you push?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666941336
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
> free should never be called in the interrupt context, so I add DEBUGASSERT(up_interrupt_context() == false) in mm_takesemaphore and remove the check here.
@xiaoxiang781216
This worries me. I have a recollection of the delayed free coming in. (it broke large start ups scripts)
There had to be a reason it was there? Have you asked the original author?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667076329
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
> > free should never be called in the interrupt context, so I add DEBUGASSERT(up_interrupt_context() == false) in mm_takesemaphore and remove the check here.
>
> @xiaoxiang781216
>
> This worries me. I have a recollection of the delayed free coming in. (it broke large start ups scripts)
>
> There had to be a reason it was there?
The comment already mention the reason why we can't free the memory immediately(most case is we can't free the running stack by the exiting thread self).
> Have you asked the original author?
Ask for what?
But, anyway this patch doesn't modify the behaviour of delay list, what's this patch really change is remove the support of recurive lock since it never happen and then don't need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666456666
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,27 @@ 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
+ * Take the MM mutex. This may be called 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).
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667168475
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
Ok, mm_free is restored to support the interrupt context. Please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667111217
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
If "free should never be called in the interrupt context, so I add DEBUGASSERT(up_interrupt_context() == false)" is a true statement then explain why the [original code]( https://github.com/apache/incubator-nuttx/commit/1d2396353e5775db50794c59201731baf2dcc153 ) tested for it an used the delay queue.
This PR breaks that behavior. I am asking you to verify if that was a bad change or this one is.
If we supporting free from up_interrupt_context (like post) this will be a breaking change to anyone that used it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667221378
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,41 @@ 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 */
+
+ if (up_interrupt_context())
+ {
+ /* Can't take semaphore in the interrupt handler */
+
+ return false;
+ }
+ else
+#endif
/* getpid() returns the task ID of the task at the head of the ready-to-
Review comment:
I believe this documentation needs to updated, since it still mentions the `holder` logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667076329
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
> > free should never be called in the interrupt context, so I add DEBUGASSERT(up_interrupt_context() == false) in mm_takesemaphore and remove the check here.
>
> @xiaoxiang781216
>
> This worries me. I have a recollection of the delayed free coming in. (it broke large start ups scripts)
Could you explain why or which startup scripts will be broken?
>
> There had to be a reason it was there?
The comment already mention the reason why we can't free the memory immediately(most case is we can't free the running stack by the exiting thread self).
> Have you asked the original author?
Ask for what?
But, anyway this patch doesn't modify the behaviour of delay list, what's this patch really change is remove the support of recurive lock since it never happen and then don't need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#issuecomment-877435303
No, thank for pointing out the difference, it should restore now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#issuecomment-877685546
@gustavonihei do you have any other concern to merge this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667221378
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,41 @@ 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 */
+
+ if (up_interrupt_context())
+ {
+ /* Can't take semaphore in the interrupt handler */
+
+ return false;
+ }
+ else
+#endif
/* getpid() returns the task ID of the task at the head of the ready-to-
Review comment:
@xiaoxiang781216 I believe this documentation needs to updated, since it still mentions the `holder` logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666457802
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
The memory will free automatically in all allocatinon function:
https://github.com/apache/incubator-nuttx/pull/4096/files#diff-35207c436ed9a9af44c9ac830e37df62e859142009ec7b09a24f81fb37caf4a7L107
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666400336
##########
File path: 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));
Review comment:
The statement should not be wrapped in the `DEBUGVERIFY` macro, otherwise it will be discarded when `CONFIG_DEBUG_ASSERTIONS` is disabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666460654
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
The way I read this was: Under an interrupt context use mm_add_delaylist(heap, mem) to delay free the memory.
Now what happens?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666406811
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,27 @@ 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
+ * Take the MM mutex. This may be called 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).
Review comment:
The description needs to be completely rewritten, it is not adequate for the current implementation.
Also add the documentation for the input parameters and return value.
BTW, the return value documentation may be the most suitable place to describe that scenario where the caller may fail to take the semaphore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666457802
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
The memory will free automatically in all allocation function:
https://github.com/apache/incubator-nuttx/pull/4096/files#diff-35207c436ed9a9af44c9ac830e37df62e859142009ec7b09a24f81fb37caf4a7L107
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei merged pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666822486
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
free should never be called in the interrupt context, so I add DEBUGASSERT(up_interrupt_context() == false) in mm_takesemaphore and remove the check here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666474459
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,33 @@ 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:
+ * return true if the semaphore can be taked, otherwise false.
Review comment:
```suggestion
* true if the semaphore can be taken, otherwise false.
```
nit: `return` may be omitted. Also fix a typo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666795059
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
I think what David is asking is where are we know adding the memory to the delayed list when we are in an interrupt context. Is `mm_takesemaphore() == false` enough?
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -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())
- {
Review comment:
I think what David is asking is where are we now adding the memory to the delayed list when we are in an interrupt context. Is `mm_takesemaphore() == false` enough?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666630180
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,33 @@ 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:
+ * return true if the semaphore can be taked, otherwise false.
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r667316912
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -77,29 +54,41 @@ 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 */
+
+ if (up_interrupt_context())
+ {
+ /* Can't take semaphore in the interrupt handler */
+
+ return false;
+ }
+ else
+#endif
/* getpid() returns the task ID of the task at the head of the ready-to-
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#issuecomment-877421788
Here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#discussion_r666400336
##########
File path: 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));
Review comment:
The statement should not be wrapped in the DEBUGVERIFY macro, otherwise it will be discarded when `CONFIG_DEBUG_ASSERTIONS` is disabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4096: mm: Simplify the semaphore handling
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4096:
URL: https://github.com/apache/incubator-nuttx/pull/4096#issuecomment-877689116
> @gustavonihei do you have any other concern to merge this PR?
No. @davids5 already approved, so I’ll merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org