You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "yf13 (via GitHub)" <gi...@apache.org> on 2024/05/27 04:22:51 UTC
[PR] sched/signal: reclaim freed sigaction blocks [nuttx]
yf13 opened a new pull request, #12406:
URL: https://github.com/apache/nuttx/pull/12406
## Sumaary
The sigaction block relcaimation logic is missing in upstream thus after
running ostest those memory are not freed timely. This patch attempts to
add the logic with the help of a bit field `sob` and address sorted list.
## Impact
Memory footprint after running ostest
## Testing
- rv-virt/nsh, rv-virt/nsh64
- CI checks
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#discussion_r1621255164
##########
sched/Kconfig:
##########
@@ -1546,6 +1546,13 @@ endmenu # RTOS hooks
menu "Signal Configuration"
+config SIG_PREALLOC_ACTIONS
Review Comment:
```suggestion
config SIG_ALLOC_ACTIONS
```
##########
sched/Kconfig:
##########
@@ -1546,6 +1546,13 @@ endmenu # RTOS hooks
menu "Signal Configuration"
+config SIG_PREALLOC_ACTIONS
+ int "Number of pre-allocated sigactions"
+ default 4 if DEFAULT_SMALL
Review Comment:
let's use 1 mean alloc/free directly, other value keeps previous behaviour:
1. allocate in batch
2. never return to heap
--
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
Re: [PR] sched/signal: reclaim sigaction [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2154821240
> @xiaoxiang781216, sorry for my late response as I didn't see inline comments from `gh` command line.
>
> would you teach a use case where `static-plus-dynamic` approach is improper but `never return` approach does a better job?
If the system hit a peak value, it may likely hit again in the future. So, it may fine to not return the memory to heap.
> I don't quite understand why we need `never return` designin upstream as it gives user apps chances to exhaust the system. If a system wants to avoid frequent small sized alloc/free operations from heap, pre-allocation shall help.
but you mayn't know how much memory need be preserved in all case.
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2134393784
> > what's benefit to reclaim the signal action pool? if it's important to safe the memory, it's simpler and fast to alloc/free one action from heap.
>
> @xiaoxiang781216 thanks for the comment. I watched 120*16=1920 bytes memory growth from ostest on `rv-virt/nsh`. This concerned me in the past and may scare new comers in the future. When reviewing the source, I saw comments in ostest like `/* Just exit, the system should clean up the signal handlers */` in ostest and the system doesn't do reclamation, so I added it.
>
But the change makes nxsig_release_action is slower than kmm_free. If so, why not remove the pool and call kmm_malloc/kmm_free direcctly?
> On the other side, can I learn the benefit of this pool over the direct alloc/free of single entries approach? how the pool size is controlled? Why this pool doesn't use flags ` sigalloc_e` like other ones?
If you want to save the memory, please study how net conn manage the socket handle:
https://github.com/apache/nuttx/blob/master/net/icmp/icmp_conn.c#L77-L204
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2134165872
@xiaoxiang781216 thanks for the comment.
I watched 120*16=1920 bytes memory growth from ostest on `rv-virt/nsh`. This concerned me in the past and may scare new comers in the future. When reviewing the source, I saw comments in ostest like `/* Just exit, the system should clean up the signal handlers */` in ostest and the system doesn't do reclamation, so I added it.
On the other side, can I learn the benefit of this pool over the direct alloc/free of single entries approach? how the pool size is controlled? Why this pool doesn't use flags ` sigalloc_e` like other ones?
--
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
Re: [PR] sched/signal: reclaim sigaction [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#discussion_r1632268682
##########
sched/signal/sig_initialize.c:
##########
@@ -166,6 +173,13 @@ void nxsig_initialize(void)
sq_init(&g_sigpendingsignal);
sq_init(&g_sigpendingirqsignal);
+#if CONFIG_SIG_PREALLOC_ACTIONS > 0
+ for (i = 0; i < CONFIG_SIG_PREALLOC_ACTIONS; i++)
+ {
+ sq_addlast((FAR sq_entry_t *)(g_sigactions + i), &g_sigfreeaction);
Review Comment:
add a function like nxsig_init_pendingsignalblock
##########
sched/signal/sig_initialize.c:
##########
@@ -55,6 +55,10 @@ struct sigpool_s
sq_queue_t g_sigfreeaction;
+#if CONFIG_SIG_PREALLOC_ACTIONS > 0
+sigactq_t g_sigactions[CONFIG_SIG_PREALLOC_ACTIONS];
Review Comment:
merge to sigpool_s
##########
sched/signal/sig_action.c:
##########
@@ -102,7 +102,7 @@ static FAR sigactq_t *nxsig_alloc_action(void)
/* Check if we got one. */
- while (!sigact)
+ if (!sigact)
Review Comment:
keep while loop, there are chance other thread grab the free action after line 109 and before line 113
##########
sched/signal/signal.h:
##########
@@ -111,6 +111,15 @@ typedef struct sigq_s sigq_t;
extern sq_queue_t g_sigfreeaction;
+#if CONFIG_SIG_PREALLOC_ACTIONS > 0
+extern sigactq_t g_sigactions[CONFIG_SIG_PREALLOC_ACTIONS];
+#define IS_PREALLOC_ACTION(x) ( \
Review Comment:
move to source file
##########
sched/signal/sig_action.c:
##########
@@ -67,12 +67,12 @@ static void nxsig_alloc_actionblock(void)
/* Allocate a block of signal actions */
- sigact = kmm_malloc((sizeof(sigactq_t)) * NUM_SIGNAL_ACTIONS);
Review Comment:
remove NUM_SIGNAL_ACTIONS
--
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
Re: [PR] sched/signal: reclaim sigaction [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#discussion_r1640562394
##########
sched/signal/sig_action.c:
##########
@@ -102,7 +102,7 @@ static FAR sigactq_t *nxsig_alloc_action(void)
/* Check if we got one. */
- while (!sigact)
+ if (!sigact)
Review Comment:
thanks, will do.
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2134164091
@xiaoxiang781216 thanks for the comment.
I watched 120*16=1920 bytes memory growth from ostest on `rv-virt/nsh`. This concerned me in the past and may scare new comers in the future. When reviewing the source, I saw comments in ostest like `/* Just exit, the system should clean up the signal handlers */` in ostest and the system doesn't do reclamation, so I added it.
On the other side, can I learn how the pool size is controlled? Why this pool doesn't use flags ` sigalloc_e` like other ones?
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#discussion_r1616207706
##########
sched/signal/sig_action.c:
##########
@@ -407,11 +447,65 @@ int sigaction(int signo, FAR const struct sigaction *act,
void nxsig_release_action(FAR sigactq_t *sigact)
{
+ FAR sq_queue_t * const list = &g_sigfreeaction;
+ FAR sigactq_t *prev;
+ FAR sigactq_t *next;
irqstate_t flags;
+ int i;
- /* Just put it back on the free list */
+ /* Put back to the free list */
+
+ nxsig_add_free(sigact);
Review Comment:
what's benefit to reclaim the signal action pool? if it's important to save the memory, it's simpler and fast to alloc/free one action from heap.
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2143350567
@xiaoxiang781216, sorry for my late response as I didn't see inline comments from `gh` command line.
would you teach a use case where `static-plus-dynamic` approach is improper but `never return` approach does a better job?
--
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
Re: [PR] sched/signal: reclaim sigaction [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2164098942
@xiaoxiang781216 thanks for the comments.
when actionblock is makde part of the private `g_sigpool`, then shall we add a function `is_prealloc_action()` to help sig_action.c? or shall we move `struct sigpool_s` to signal.h and make g_sigpool non-private? The latter looks more efficient.
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2136408910
> But the change makes nxsig_release_action is slower than kmm_free. If so, why not remove the pool and call kmm_malloc/kmm_free direcctly?
> please study how icmp manage the socket conn:
@xiaoxiang781216 thanks for the pointers.
how about having pre-allocated entries (defined by `SIG_PREALLOC_ACTIONS`) and dynamic entries from direct `kmm_malloc/kmm_free`?
--
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
Re: [PR] sched/signal: reclaim sigaction blocks [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#discussion_r1616207706
##########
sched/signal/sig_action.c:
##########
@@ -407,11 +447,65 @@ int sigaction(int signo, FAR const struct sigaction *act,
void nxsig_release_action(FAR sigactq_t *sigact)
{
+ FAR sq_queue_t * const list = &g_sigfreeaction;
+ FAR sigactq_t *prev;
+ FAR sigactq_t *next;
irqstate_t flags;
+ int i;
- /* Just put it back on the free list */
+ /* Put back to the free list */
+
+ nxsig_add_free(sigact);
Review Comment:
what's benefit to reclaim the signal action pool? if it's important to safe the memory, it's simpler and fast to alloc/free one action from heap.
--
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
Re: [PR] sched/signal: reclaim sigaction [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #12406:
URL: https://github.com/apache/nuttx/pull/12406#issuecomment-2156213409
@acassis, it seems [this job log](https://github.com/apache/nuttx/actions/runs/9431629406/job/25980516627?pr=12406) complains about the `stm3240g-eval/knxwm` config:
```
arm-none-eabi-ld: /github/workspace/sources/nuttx/nuttx section `.bss' will not fit in region `ksram'
arm-none-eabi-ld: region `ksram' overflowed by 72 bytes
```
I am wondering if we should enlarge the "sram" region in linker script a little?
--
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