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/11/04 13:42:32 UTC
[GitHub] [incubator-nuttx] GUIDINGLI opened a new pull request #4782: sem: remove limitation of irq context when do sem_trywait
GUIDINGLI opened a new pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782
## Summary
sem: remove limitation of irq context when do sem_trywait
## Impact
allow mm_takesemaphore in IRQ, just try get
## Testing
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-963799515
> Is this a good idea? Allocating memory in an interrupt context can effect latency and add jitter.
> This will actually make the `free` part succeed,
The intent of change is allowing https://github.com/apache/incubator-nuttx/pull/4759 can do the check even in the interrupt context. So, malloc/free shouldn't call from the interrupt context.
> and we will be freeing memory if we successfully get the lock on the first try. `malloc` in the other hand only `DEBUGVERIFY` the check, when debug options are disabled we are then allocating memory and possibly without holding the lock?
How about we add DEBUGASSERT in malloc/free to catch the wrong usage?
--
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 pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-961748337
This will actually make the `free` part succeed, and we will be freeing memory if we successfully get the lock on the first try. `malloc` in the other hand only `DEBUGVERIFY` the check, when debug options are disabled we are then allocating memory and possibly without holding the lock?
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r745640174
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -86,6 +86,10 @@ void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
kasan_poison(mem, mm_malloc_size(mem));
+ /* Must not in IRQ */
+
+ DEBUGASSERT(!up_interrupt_context());
Review comment:
In the case we are in an interrupt context, this will nullify the changes we have in this PR for `mm_takesemaphore`.
This check seems to be already implemented by the usual `DEBUGVERIFY(mm_takesemaphore)`.
--
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] GUIDINGLI commented on a change in pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r748350090
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
Then, there is one question, why user can't call nxsem_trywait from interrupt context ?
There is no harmful, even on SMP mode.
nxsem_trywait only look at the sem count, it will NOT wait all.
If someone use nxsem_trywait, and nxsem_post before the IRQ exit, that's the normal thing I think.
--
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 merged pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782
--
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] GUIDINGLI commented on pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-961683498
--
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 pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-961041979
Is this a good idea? Allocating memory in an interrupt context can effect latency and add jitter.
--
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 pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-961748337
This will actually make the `free` part succeed, and we will be freeing memory if we successfully get the lock on the first try. `malloc` in the other hand only `DEBUGVERIFY` the check, when debug options are disabled we are then allocating memory and possibly without holding the lock?
--
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 edited a comment on pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-963799515
> Is this a good idea? Allocating memory in an interrupt context can effect latency and add jitter.
> This will actually make the `free` part succeed,
The intent of change is allowing https://github.com/apache/incubator-nuttx/pull/4759 can do the check even in the interrupt context.
> and we will be freeing memory if we successfully get the lock on the first try. `malloc` in the other hand only `DEBUGVERIFY` the check, when debug options are disabled we are then allocating memory and possibly without holding the lock?
How about we add DEBUGASSERT in malloc/free to catch the wrong usage?
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r745573479
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -86,6 +86,10 @@ void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
kasan_poison(mem, mm_malloc_size(mem));
+ /* Must not in IRQ */
+
+ DEBUGVERIFY(!up_interrupt_context());
Review comment:
Is the condition here correct? Or `DEBUGASSERT` was meant to be used?
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r746684541
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
Why not leave this wrapped in `ifndef CONFIG_DEBUG_MM` ?
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r745839264
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -86,6 +86,10 @@ void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
kasan_poison(mem, mm_malloc_size(mem));
+ /* Must not in IRQ */
+
+ DEBUGASSERT(!up_interrupt_context());
Review comment:
I was actually commenting about this line for all other files. But it looks like I was wrong, I was thinking about `free` while the idea of this change is to use `mm_checkcorruption` in an interrupt context.
--
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] GUIDINGLI commented on a change in pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r750424662
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
@davids5 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] GUIDINGLI commented on a change in pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r748350090
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
Then, there is one question, why user can't call `nxsem_trywait` from interrupt context ?
There is no harmful, even on SMP mode.
`nxsem_trywait` only look at the sem count, it will NOT wait at all.
If someone use `nxsem_trywait`, and `nxsem_post` before the IRQ exit, that's the normal thing I think.
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r747511117
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
I see your point. But config.h is global AND you have removed a protection (to facilitate a DEBUG only feature)!
My concern would for a caller of nxsem_trywait (direct) and now the protection is gone. Maybe that does not matter, but if you add
```
#ifdnef CONFIG_DEBUG_MM
/* This API should not be called from interrupt handlers */
DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
#endif
```
It would help to debug bad calling sequence. Is the only objection having the condition checked CONFIG_DEBUG_MM in
sched/semaphore/sem_trywait.c? Have you considered that the cosmetics is minor compared to the debug time that would be spent?
--
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 pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-961748337
This will actually make the `free` part succeed, and we will be freeing memory if we successfully get the lock on the first try. `malloc` in the other hand only `DEBUGVERIFY` the check, when debug options are disabled we are then allocating memory and possibly without holding the lock?
--
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 pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-964070575
> > Is this a good idea? Allocating memory in an interrupt context can effect latency and add jitter.
> > This will actually make the `free` part succeed,
>
> The intent of change is allowing #4759 can do the check even in the interrupt context.
>
> > and we will be freeing memory if we successfully get the lock on the first try. `malloc` in the other hand only `DEBUGVERIFY` the check, when debug options are disabled we are then allocating memory and possibly without holding the lock?
>
> How about we add DEBUGASSERT in malloc/free to catch the wrong usage?
Why not make the mm/mm_heap/mm_sem.c change dependent on CONFIG_DEBUG_MM?
Effectively the code is left as is when CONFIG_DEBUG_MM is not defined and as in this PR when it is defined?
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r745570582
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -102,7 +103,7 @@ bool mm_takesemaphore(FAR struct mm_heap_s *heap)
return false;
}
#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
- else if (sched_idletask())
+ else if (sched_idletask() || up_interrupt_context())
Review comment:
It looks like this path is also taken when `CONFIG_DEBUG_MM` is disabled and we are in an interrupt context.
Maybe if we pull the interrupt handling aside the code would look simpler, like:
```c
#if (defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)))
/* Check current environment */
if (up_interrupt_context())
{
# ifdef CONFIG_DEBUG_MM
return _SEM_TRYWAIT(&heap->mm_semaphore) >= 0;
# else
/* Can't take semaphore in the interrupt handler */
return false;
#endif
}
else
#endif
```
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r747113311
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
@xiaoxiang781216 Simple is always better. If all code path lead to the one check that would be preferred. If not add them where needed.
--
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] GUIDINGLI commented on pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-964085424
@davids5 Good idea! 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] GUIDINGLI commented on a change in pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r745699316
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -86,6 +86,10 @@ void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
kasan_poison(mem, mm_malloc_size(mem));
+ /* Must not in IRQ */
+
+ DEBUGASSERT(!up_interrupt_context());
Review comment:
So you mean remove this check in mm_free() ?
But in order to align the code with mm_malloc(), then suggest add this check
--
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] GUIDINGLI commented on pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-964751011
@davids5 Please review the newest version
--
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] GUIDINGLI commented on a change in pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r746213754
##########
File path: mm/mm_heap/mm_free.c
##########
@@ -86,6 +86,10 @@ void mm_free(FAR struct mm_heap_s *heap, FAR void *mem)
kasan_poison(mem, mm_malloc_size(mem));
+ /* Must not in IRQ */
+
+ DEBUGASSERT(!up_interrupt_context());
Review comment:
Add the purpose to Summary
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r746864219
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
Since the behavior change happen only when CONFIG_DEBUG_MM is defined, we can simply to:
1. #ifndef CONFIG_DEBUG_MM the below debug assert
2. keep _SEM_TRYWAIT in mm/mm_heap/mm_sem.c with CONFIG_DEBUG_MM guard
3. Remove all other assert
Do you think so? @davids5 .
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r747511117
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
I see your point. But config.h is global AND you have removed a protection (to facilitate a DEBUG only feature)!
My concern would for a caller of nxsem_trywait (direct) and now the protection is gone. Maybe that does not matter, but if you add
```
#ifdnef CONFIG_DEBUG_MM
/* This API should not be called from interrupt handlers */
DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
#endif
```
It would help to debug bad calling sequence. Is that only objection having the condition checked CONFIG_DEBUG_MM in
sched/semaphore/sem_trywait.c? Have you considered that the cosmetics is minor compared to the debug time that would be spent?
--
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] GUIDINGLI commented on a change in pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r747174852
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
@davids5 Directly remove is the simply way to sem_trywait(), use CONFIG_DEBUG_MM under `sched` folder is NOT suitable. I strongly suggest keep current code.
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r748356054
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
ehhh...what is the value of FAR struct tcb_s *rtcb = this_task(); in the ISR?
--
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 #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#discussion_r747544613
##########
File path: sched/semaphore/sem_trywait.c
##########
@@ -69,10 +69,6 @@ int nxsem_trywait(FAR sem_t *sem)
irqstate_t flags;
int ret;
- /* This API should not be called from interrupt handlers */
Review comment:
(Unresolving this for visibility)
I had the same concern as David's, we will lose the possibility of catching calls from interrupt context to `nxsem_trywait`.
--
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] GUIDINGLI commented on pull request #4782: sem: remove limitation of irq context when do sem_trywait
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #4782:
URL: https://github.com/apache/incubator-nuttx/pull/4782#issuecomment-961683498
NOT alloc memory, just get the mm lock to check corruption
--
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