rename lib_pathbuffer to lib_tempbuffer #15326

Zhangshoukui · 2024-12-24T09:23:07Z

Summary

rename lib_pathbuffer to lib_tempbuffer

Impact

None

Testing

./tools/configure.sh -l sim:nsh
make -j7

Signed-off-by: zhangshoukui <[email protected]>

…CONFIG_LINE_MAX Signed-off-by: zhangshoukui <[email protected]>

nuttxpr · 2024-12-24T09:31:19Z

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details.

Here's what's missing:

Summary: It states what is being changed (renaming lib_pathbuffer to lib_tempbuffer), but not why. What problem does this renaming solve? What was the motivation for this change? How does the renaming work (is it a simple rename, or are there code changes associated with it)? Are there related NuttX issues?
Impact: Simply stating "None" is insufficient. Even a seemingly simple rename can have unforeseen impacts. The PR needs to explicitly address each impact point with justification. For example, why is there no impact on documentation? If there truly is no impact, explicitly state "NO" for each item.
Testing: While it mentions build commands, it provides no actual testing logs. Showing the output before and after the change is crucial to demonstrate that the change works as intended and doesn't introduce regressions. What functionality was tested? What were the expected results? Also, specifying only the build host is insufficient; the target architecture (e.g., sim:nsh) should be listed under Target(s) with details about the board and configuration. Furthermore, mentioning the build host operating system, CPU architecture, and compiler version is essential.

In short, the PR needs to be significantly more detailed to meet the NuttX requirements. It needs to explain the rationale behind the change, justify the claimed lack of impact, and provide concrete evidence of testing.

anchao · 2024-12-24T12:52:23Z

libs/libc/misc/lib_tempbuffer.c

 {
-  for (; ; )
+  if (nbytes <= TEMP_MAX_SIZE)


I don't understand this change. If the required path buffer size is large than PATH_MAX, why not just call malloc() in application? Why must use this API?
I haven't seen lib_get_pathbuffer() used anywhere else except the kernel, 100% of the source code is using PATH_MAX, why add this redundant check?

nbytes <= TEMP_MAX_SIZE: Make sure don't cross the line

At present, parameter passing is supported, and the code under the following app uses this interface to optimize the use of the stack. The new feature of nshlib has exceeded the call stack of 2k.

You should call malloc in nshlib instead of making the interface so ugly

many places (especially in nsh) need allocate temp buffer (e.g. path or line) only in function scope, both follow approach isn't perfect:

Allocate from stack, which already cause the default 2KB stack is unusable in many cases:
{bp-15165} rv-virt/citest: Increase init task stack size to 3072 #15173
esp32s3: Increase the init task stask size when using NSH #14501

Allocate from heap, which may increase memory fragment and impact performance

so, it's reasonable to extend pathbuffer to tempbuffer, and let's caller utilize the general buffer facility when they find some function occupy too much stack space.

You should call malloc in nshlib instead of making the interface so ugly

It's natural to add size argument to a buffer allocation function. Do you feel ugly malloc with size or not strange malloc without size?

The implementation of pathbuffer is a memory pool with a limited length. Currently, all the kernel code is using the length of PATH_MAX. From the kernel perspective, add length parameter is a strange design, because the kernel code can ensure that all locations(100%) that call pathbuffer only use PATH_MAX:

FAR char *lib_get_pathbuffer(void);
-> FAR char *lib_get_tempbuffer(size_t nbytes); <- why?

If the customized length in the application exceeds PATH_MAX, we should manage the life cycle of this memory segment by application self. If the stack occupies too much, we can choose to define it statically or call malloc:

https://github.com/apache/nuttx-apps/blob/master/nshlib/nsh_console.h#L157

The design of the API should be more reasonable and easy to use. If 99% of the code is using PATH_MAX as the expected value, why should these codes bear the overhead of parameter passing and parameter checking? This is unreasonable

i'm not sure how it's different from malloc either.

Allocate from heap, which may increase memory fragment and impact performance

do you mean line buffer allocation in nsh is performance critical? really?
also, as far as the allocation lifetime is very short, fragmentation is not a big concern i suspect.

xiaoxiang781216 · 2024-12-24T16:14:24Z

include/nuttx/lib/lib.h

@@ -38,6 +38,12 @@
 * Pre-processor Definitions
 ****************************************************************************/

+#if CONFIG_PATH_MAX > CONFIG_LINE_MAX


keep in the soure code

xiaoxiang781216 · 2024-12-24T16:17:15Z

include/nuttx/lib/lib.h

 #else
-#  define lib_get_pathbuffer() alloca(PATH_MAX)
-#  define lib_put_pathbuffer(b)
+#  define lib_get_tempbuffer(f) alloca(TEMP_MAX_SIZE)


# define lib_get_tempbuffer(n) alloca(n)

xiaoxiang781216 · 2024-12-24T16:37:20Z

libs/libc/misc/lib_tempbuffer.c

 {
-  for (; ; )
+  if (nbytes <= TEMP_MAX_SIZE)


many places (especially in nsh) need allocate temp buffer (e.g. path or line) only in function scope, both follow approach isn't perfect:

Allocate from stack, which already cause the default 2KB stack is unusable in many cases:
{bp-15165} rv-virt/citest: Increase init task stack size to 3072 #15173
esp32s3: Increase the init task stask size when using NSH #14501

Allocate from heap, which may increase memory fragment and impact performance

so, it's reasonable to extend pathbuffer to tempbuffer, and let's caller utilize the general buffer facility when they find some function occupy too much stack space.

You should call malloc in nshlib instead of making the interface so ugly

It's natural to add size argument to a buffer allocation function. Do you feel ugly malloc with size or not strange malloc without size?

acassis

@Zhangshoukui I think we need to simplify the LINELEN / LINEMAX instead of creating a new one, currently there are many configs to that:

CONFIG_SYSTEM_CLE_CMD_HISTORY_LINELEN
CONFIG_SYSTEM_CLE_CMD_HISTORY_LEN
CONFIG_NSH_LINELEN
CONFIG_PROCFS_LINELEN
CONFIG_HELP_LINELEN

Having a LINELEN for each application could be more flexible, but it is also a lot a code duplication. It should be nice to have a single LINE_MAX to be the standard LINELEN

Zhangshoukui · 2024-12-25T07:28:01Z

@Zhangshoukui I think we need to simplify the LINELEN / LINEMAX instead of creating a new one, currently there are many configs to that:

CONFIG_SYSTEM_CLE_CMD_HISTORY_LINELEN CONFIG_SYSTEM_CLE_CMD_HISTORY_LEN CONFIG_NSH_LINELEN CONFIG_PROCFS_LINELEN CONFIG_HELP_LINELEN

Having a LINELEN for each application could be more flexible, but it is also a lot a code duplication. It should be nice to have a single LINE_MAX to be the standard LINELEN

@acassis Yes, the next step is to optimize and remove the above configuration to use LINE_MAX.
remove this:
config SYSTEM_CLE_CMD_HISTORY_LEN
config SYSTEM_CLE_CMD_HISTORY_LINELEN
CONFIG_NSH_LINELEN
HELP_LINELEN
PROCFS_LINELEN
cc @anchao

yamt · 2024-12-25T07:34:46Z

fs/littlefs/lfs_vfs.c

@@ -1531,7 +1531,7 @@ static int littlefs_mkdir(FAR struct inode *mountpt, FAR const char *relpath,

  if (len > 0 && relpath[len - 1] == '/')
    {
-      path = lib_get_pathbuffer();
+      path = lib_get_tempbuffer(PATH_MAX);


i feel it's simpler to make

#define lib_get_pathbuffer() lib_get_tempbuffer(PATH_MAX)

instead of modifying all callers.

i feel it's simpler to make

#define lib_get_pathbuffer() lib_get_tempbuffer(PATH_MAX)

instead of modifying all callers.

that way we can go back to the fixed-sized allocataion easily if/when this change turned out to be a bad idea. :-)

yamt · 2024-12-25T07:40:10Z

libs/libc/misc/lib_tempbuffer.c

 {
-  for (; ; )
+  if (nbytes <= TEMP_MAX_SIZE)


i'm not sure how it's different from malloc either.

Allocate from heap, which may increase memory fragment and impact performance

do you mean line buffer allocation in nsh is performance critical? really?
also, as far as the allocation lifetime is very short, fragmentation is not a big concern i suspect.

yamt · 2024-12-25T07:41:19Z

sched/Kconfig

+	default 64 if DEFAULT_SMALL
+	default 80 if !DEFAULT_SMALL
+	---help---
+		The maximum size of line.


please add explanation about what "line" here is.

Signed-off-by: zhangshoukui <[email protected]>

Zhangshoukui added 2 commits December 24, 2024 15:23

Add LINE_MAX configuration

a56b723

Signed-off-by: zhangshoukui <[email protected]>

lib_get_pathbuffer: Modify size to the maximum value of PATH_MAX and …

9412f95

…CONFIG_LINE_MAX Signed-off-by: zhangshoukui <[email protected]>

Zhangshoukui force-pushed the temp_ branch from 017e4be to 1f0d3f2 Compare December 24, 2024 12:28

anchao requested changes Dec 24, 2024

View reviewed changes

xiaoxiang781216 reviewed Dec 24, 2024

View reviewed changes

Zhangshoukui force-pushed the temp_ branch from 1f0d3f2 to 1216aa6 Compare December 25, 2024 01:49

acassis reviewed Dec 25, 2024

View reviewed changes

yamt reviewed Dec 25, 2024

View reviewed changes

rename lib_pathbuffer to lib_tempbuffer

24c37e2

Signed-off-by: zhangshoukui <[email protected]>

Zhangshoukui force-pushed the temp_ branch from 1216aa6 to 24c37e2 Compare December 25, 2024 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename lib_pathbuffer to lib_tempbuffer #15326

rename lib_pathbuffer to lib_tempbuffer #15326

Zhangshoukui commented Dec 24, 2024

nuttxpr commented Dec 24, 2024

anchao Dec 24, 2024

Zhangshoukui Dec 24, 2024

anchao Dec 24, 2024

xiaoxiang781216 Dec 24, 2024

anchao Dec 25, 2024 •

edited

Loading

yamt Dec 25, 2024

xiaoxiang781216 Dec 24, 2024

xiaoxiang781216 Dec 24, 2024

xiaoxiang781216 Dec 24, 2024

acassis left a comment

Zhangshoukui commented Dec 25, 2024 •

edited

Loading

yamt Dec 25, 2024

yamt Dec 25, 2024

yamt Dec 25, 2024

yamt Dec 25, 2024

Zhangshoukui Dec 25, 2024

rename lib_pathbuffer to lib_tempbuffer #15326

Are you sure you want to change the base?

rename lib_pathbuffer to lib_tempbuffer #15326

Conversation

Zhangshoukui commented Dec 24, 2024

Summary

Impact

Testing

nuttxpr commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anchao Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acassis left a comment

Choose a reason for hiding this comment

Zhangshoukui commented Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anchao Dec 25, 2024 •

edited

Loading

Zhangshoukui commented Dec 25, 2024 •

edited

Loading