You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "crafcat7 (via GitHub)" <gi...@apache.org> on 2024/11/04 09:30:28 UTC
[PR] Use pathbuffer reduce stack [nuttx]
crafcat7 opened a new pull request, #14632:
URL: https://github.com/apache/nuttx/pull/14632
## Summary
**Why is this change necessary?**
Benefit from the lib_pathbuffer feature introduced in https://github.com/apache/nuttx/pull/12969, we can further optimize the stack usage size in the NuttX kernel.
**Changes**
In this change, we replaced the declarations related to xxx[PATH_MAX] in the NuttX directory with the lib_pathbuffer method
## Impact
**Is a new feature added?:** NO
**Impact on build:** NO
**Impact on hardware:** NO, this change does not specifically target any particular hardware architectures or boards.
**Impact on documentation:** NO,This patch does not introduce any new features
**Impact on compatibility:** This implementation aims to be backward compatible. No existing functionality is expected to be broken.
## Testing
Build Host(s): Linux x86
Target(s): sim/nsh
By default
```
LIBC_MAX_PATHBUFFER = 2
LIBC_PATHBUFFER_MALLOC = y
```
So when there is not enough memory on the stack, it will be requested from the 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] Use pathbuffer reduce stack [nuttx]
Posted by "crafcat7 (via GitHub)" <gi...@apache.org>.
crafcat7 commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2458976780
In CI, it is found that when executing kernel build in non-kernel scenarios, builtin_ffs will not be found, but `CONFIG_HAVE_BUILTIN_FFS` will be defined
```
riscv-none-elf-ld: /github/workspace/sources/nuttx/staging/libc.a(lib_pathbuffer.o): in function `lib_get_pathbuffer':
/github/workspace/sources/nuttx/libs/libc/misc/lib_pathbuffer.c:89:(.text.lib_get_pathbuffer+0x1c): undefined reference to `ffs'
make[1]: *** [Makefile:189: nuttx] Error 1
make: *** [tools/Unix.mk:551: nuttx] Error 2
```
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#discussion_r1835562863
##########
libs/libc/misc/lib_pathbuffer.c:
##########
@@ -87,7 +87,7 @@ FAR char *lib_get_pathbuffer(void)
/* Try to find a free buffer */
nxmutex_lock(&g_pathbuffer.lock);
- index = ffs(g_pathbuffer.free_bitmap) - 1;
+ index = ffsl(g_pathbuffer.free_bitmap) - 1;
Review Comment:
change line 45 to `unsigned long free_bitmap;`
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "crafcat7 (via GitHub)" <gi...@apache.org>.
crafcat7 commented on code in PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#discussion_r1835680898
##########
libs/libc/misc/lib_pathbuffer.c:
##########
@@ -87,7 +87,7 @@ FAR char *lib_get_pathbuffer(void)
/* Try to find a free buffer */
nxmutex_lock(&g_pathbuffer.lock);
- index = ffs(g_pathbuffer.free_bitmap) - 1;
+ index = ffsl(g_pathbuffer.free_bitmap) - 1;
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
Re: [PR] Use pathbuffer reduce stack [nuttx]
Posted by "crafcat7 (via GitHub)" <gi...@apache.org>.
crafcat7 commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2472199593
> Sorry @crafcat7 I think `spresense/example_camera` is failing because of this PR:
>
> ```
> Configuration/Tool: spresense/example_camera,CONFIG_ARM_TOOLCHAIN_GNU_EABI
> mount/fs_automount.c: In function 'automount_initialize':
> Error: mount/fs_automount.c:816:7: error: 'return' with no value, in function returning non-void [-Werror=return-type]
> 816 | return;
> | ^~~~~~
> In file included from mount/fs_automount.c:43:
> /github/workspace/sources/nuttx/include/nuttx/fs/automount.h:176:11: note: declared here
> 176 | FAR void *automount_initialize(FAR const struct automount_lower_s *lower);
> | ^~~~~~~~~~~~~~~~~~~~
> ```
>
> https://github.com/NuttX/nuttx/actions/runs/11796808033/job/32859585604#step:7:338
Okay, I submitted a fix on the https://github.com/apache/nuttx/pull/14752
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#discussion_r1827590963
##########
arch/risc-v/src/common/espressif/esp_timer.c:
##########
@@ -539,7 +540,13 @@ int esp_timer_initialize(uint32_t timer_id)
break;
}
- snprintf(devpath, sizeof(devpath), "/dev/timer%" PRIu32, timer_id);
+ devpath = lib_get_pathbuffer();
Review Comment:
let us add a configure `CONFIG_LIBC_PATHBUFFER` to enable/disable lib_get_pathbuffer implement.
```
#ifdef CONFIG_LIBC_PATHBUFFER
FAR char *lib_get_pathbuffer(void);
#else
# define lib_get_pathbuffer() alloca(PATH_MAX)
#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
Re: [PR] Use pathbuffer reduce stack [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #14632:
URL: https://github.com/apache/nuttx/pull/14632
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#discussion_r1827843019
##########
arch/risc-v/src/common/espressif/esp_timer.c:
##########
@@ -539,7 +540,13 @@ int esp_timer_initialize(uint32_t timer_id)
break;
}
- snprintf(devpath, sizeof(devpath), "/dev/timer%" PRIu32, timer_id);
+ devpath = lib_get_pathbuffer();
Review Comment:
alloca isn't ANSI or POSIX Api, so it isn't available on all possible compiler.
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "lupyuen (via GitHub)" <gi...@apache.org>.
lupyuen commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2471691545
@xiaoxiang781216 That's because the [CI Check for this Complex PR](https://github.com/apache/nuttx/actions/runs/11772192641) doesn't include arm-01, which contains `spresense/example_camera`. We [swapped arm-01 for arm-13](https://lupyuen.codeberg.page/articles/ci3.html#halve-the-ci-checks) some weeks ago.
#14748 is a [Simple Arm32 PR](https://lupyuen.codeberg.page/articles/ci3.html#halve-the-ci-checks:~:text=When%20we%20create%20a%20Simple%20PR%20for%20Arm32%3A%20It%20will%20trigger%20only%20the%20CI%20Checks%20for%20arm%2D01%20%E2%80%A6%20arm%2D14.) that will run CI Checks for all Arm32 Targets: arm-01 to arm14.
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2454220055
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
Yes, this PR appears to meet the NuttX requirements based on the provided information. However, there are a couple of minor improvements that could be made to strengthen the testing section.
While the PR states testing was done on `sim/nsh`, the "Testing logs before change" and "Testing logs after change" sections are empty. Providing concrete examples of the stack usage before and after the change would significantly improve the review process. For example, showing the output of a command that displays stack usage, or even a code snippet demonstrating how the stack size was measured would be beneficial. This would provide concrete evidence of the optimization.
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "lupyuen (via GitHub)" <gi...@apache.org>.
lupyuen commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2470511025
Sorry @crafcat7 I think `spresense/example_camera` is failing because of this PR:
```text
Configuration/Tool: spresense/example_camera,CONFIG_ARM_TOOLCHAIN_GNU_EABI
mount/fs_automount.c: In function 'automount_initialize':
Error: mount/fs_automount.c:816:7: error: 'return' with no value, in function returning non-void [-Werror=return-type]
816 | return;
| ^~~~~~
In file included from mount/fs_automount.c:43:
/github/workspace/sources/nuttx/include/nuttx/fs/automount.h:176:11: note: declared here
176 | FAR void *automount_initialize(FAR const struct automount_lower_s *lower);
| ^~~~~~~~~~~~~~~~~~~~
```
https://github.com/NuttX/nuttx/actions/runs/11796808033/job/32859585604#step:7:338
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#discussion_r1827843019
##########
arch/risc-v/src/common/espressif/esp_timer.c:
##########
@@ -539,7 +540,13 @@ int esp_timer_initialize(uint32_t timer_id)
break;
}
- snprintf(devpath, sizeof(devpath), "/dev/timer%" PRIu32, timer_id);
+ devpath = lib_get_pathbuffer();
Review Comment:
alloca isn't ANSI or POSIX Api, so it isn't available on all possible compiler, and should disable by default.
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#discussion_r1827590963
##########
arch/risc-v/src/common/espressif/esp_timer.c:
##########
@@ -539,7 +540,13 @@ int esp_timer_initialize(uint32_t timer_id)
break;
}
- snprintf(devpath, sizeof(devpath), "/dev/timer%" PRIu32, timer_id);
+ devpath = lib_get_pathbuffer();
Review Comment:
Since `CONFIG_PATH_MAX` can define a smaller size. let us add a configure `CONFIG_LIBC_PATHBUFFER` to enable/disable lib_get_pathbuffer implement.
```
#ifdef CONFIG_LIBC_PATHBUFFER
FAR char *lib_get_pathbuffer(void);
#else
# define lib_get_pathbuffer() alloca(PATH_MAX)
#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
Re: [PR] Use pathbuffer reduce stack [nuttx]
Posted by "crafcat7 (via GitHub)" <gi...@apache.org>.
crafcat7 commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2466714695
But the same thing happens on riscv's ci: the error message "undefined reference to `ffs'" still appears.
Is there something wrong with the riscv related configuration?
```
====================================================================================
Configuration/Tool: ox64/spi
2024-11-09 15:15:26
------------------------------------------------------------------------------------
Cleaning...
Configuring...
Building NuttX...
riscv-none-elf-ld: /github/workspace/sources/nuttx/staging/libkarch.a(bl808_irq.o): in function `ffs':
/github/workspace/sources/nuttx/include/strings.h:82:(.text.__m0ic_interrupt+0x1e): undefined reference to `ffs'
riscv-none-elf-ld: /github/workspace/sources/nuttx/include/strings.h:82:(.text.__m0ic_interrupt+0x30): undefined reference to `ffs'
make[1]: *** [Makefile:189: nuttx] Error 1
make: *** [tools/Unix.mk:551: nuttx] Error 2
make: Target 'all' not remade because of errors.
/github/workspace/sources/nuttx/tools/testbuild.sh: line 385: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
Normalize ox64/spi
HEAD detached at pull/14632/merge
Untracked files:
(use "git add <file>..." to include in what will be committed)
boards/risc-v/bl808/ox64/scripts/ld.script.tmp
nothing added to commit but untracked files present (use "git add" to track)
```
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#discussion_r1828574259
##########
arch/risc-v/src/common/espressif/esp_timer.c:
##########
@@ -539,7 +540,13 @@ int esp_timer_initialize(uint32_t timer_id)
break;
}
- snprintf(devpath, sizeof(devpath), "/dev/timer%" PRIu32, timer_id);
+ devpath = lib_get_pathbuffer();
Review Comment:
Current code base:
https://github.com/apache/nuttx/blob/master/include/string.h#L40-L51
I think most toolchains support this feature (even MSVC has a similar implementation)
https://github.com/apache/nuttx/blob/master/include/alloca.h#L30-L42
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "crafcat7 (via GitHub)" <gi...@apache.org>.
crafcat7 commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2469517650
Change `ffs` in `lib_getpathbuffer` to `ffsl` that everything will be OK
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2471313168
@lupyuen do you know why ci in this pr doesn't catch this error, but https://github.com/apache/nuttx/pull/14748 can.
--
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] Use pathbuffer reduce stack [nuttx]
Posted by "crafcat7 (via GitHub)" <gi...@apache.org>.
crafcat7 commented on PR #14632:
URL: https://github.com/apache/nuttx/pull/14632#issuecomment-2457109570
I submitted a fix for the redefinition problem in libc under kernel build when using ffs/ffsl/ffsll.
https://github.com/apache/nuttx/pull/14632/commits/a9e1d1bd69322041229bbb2e7fbf4d61d56e6e5f
--
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