Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "stm3240g-eval:knxwm and olimex-stm32-p407:kmodule remove FS_REFCOUNT" #13519

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Sep 18, 2024

Summary

  1. fs/reference_count: set default reference count option to n

Most of developers will not have the scenarios of open/close file descriptors in multi-threads,
The default option will incur additional code size overhead for such devices.
this PR will preserve the behavior before PR #13296 was introduced, and ensure that the default code size is not affected.

Note that this option will ensure the safety of access to the file
system from multi-tasks (Task A blocking rw(fd), then Task B close(fd)),
the disadvantage is that it will increase the amount of code-size,
there is no need to enable this option if the application could ensure
he file operations are safe.

Original:

$ size nuttx
   text	   data	    bss	    dec	    hex	filename
 479327	   8016	  10848	 498191	  79a0f	nuttx

After introduce FS_REFCOUNT:

$ size nuttx
   text	   data	    bss	    dec	    hex	filename
 479855	   8016	  10912	 498783	  79c5f	nuttx

text +528
bss +64

  1. sched/timer: move sigev_notify_thread_id to SIG_EVTHREAD

Some developers do not need this kind of advanced features,
move sigev_notify_thread_id into SIG_EVTHREAD to save the code size.

  1. Revert "olimex-stm32-p407: adjust memory layout"

This reverts commit 9a9d0a6.

  1. Revert "stm3240g-eval:knxwm and olimex-stm32-p407:kmodule remove FS_REFCOUNT"

This reverts commit 756faf3.

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ci-check

@@ -121,11 +121,17 @@ config FS_HEAPSIZE

config FS_REFCOUNT
bool "File reference count"
default !DEFAULT_SMALL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the user care size, defconfig should select DEFAULT_SMALL first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all developers are aware of DEFAULT_SMALL, this change will only keep the behavior before #13296 was introduced so that it does not cause any burden to other developers, the default configuration adds 500+ bytes per platform, which is unfair to junior developers.

#define sigev_notify_thread_id _sigev_un._tid
# define sigev_notify_function _sigev_un._sigev_thread._function
# define sigev_notify_attributes _sigev_un._sigev_thread._attribute
# define sigev_notify_thread_id _sigev_un._tid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need new Kconfig, sigev_notify_thread_id is totally different from sigev_notify_function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to add new options, which will make this feature look worse. Thread ID notification is also a kind of thread notification. Why couldn‘t use a configuration to manager them together? a other option should we change the name of CONFIG_SIG_EVTHREAD?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you drop the patch related to sigev_notify_thread_id? @Fix-Point will provide a new patch to reduce the code size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anchao #13530 could reduce the code size to avoid modifying the memory layout of olimex-stm32-p407.

Some developers do not need this kind of advanced features,
move sigev_notify_thread_id into SIG_EVTHREAD to save the code size.

Signed-off-by: chao an <[email protected]>
Most of developers will not have the scenarios of open/close file descriptors in multi-threads,
The default option will incur additional code size overhead for such devices.
this PR will preserve the behavior before PR#13296 was introduced, and ensure that the default code size is not affected.

Note that this option will ensure the safety of access to the file
system from multi-tasks (Task A blocking rw(fd), then Task B close(fd)),
the disadvantage is that it will increase the amount of code-size,
there is no need to enable this option if the application could ensure
he file operations are safe.

Signed-off-by: chao an <[email protected]>
@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirement Check

While this PR description provides some information, it does not fully meet the NuttX requirements. Here's a breakdown:

Strengths:

  • Summary:
    • Clearly states the reasons for the changes (code size optimization, reverting unnecessary features).
    • Identifies the affected code sections (fs/reference_count, sched/timer).
    • Provides some explanation of how the changes work.

Weaknesses:

  • Summary:
    • Lacks specific details about how the changes are implemented.
    • Does not reference any related NuttX issues or pull requests.
  • Impact:
    • Simply states "N/A" without addressing each impact category. This needs to be filled out thoroughly, explaining whether there is an impact (even if the answer is "NO") and providing details if applicable.
  • Testing:
    • Only mentions "ci-check" which is not informative enough.
    • Needs to specify the build hosts and target platforms used for testing.
    • Lacks testing logs before and after the changes.

Recommendations for Improvement

  1. Elaborate on Implementation Details:
    • In the "Summary" section, provide a more detailed explanation of how each change achieves its goal.
    • For example, how is the default reference count option changed? How is sigev_notify_thread_id moved to SIG_EVTHREAD?
  2. Complete the "Impact" Section:
    • Address each impact category explicitly, even if the answer is "NO".
    • Provide justification for each answer.
    • If there is any impact, describe it thoroughly.
  3. Provide Comprehensive Testing Information:
    • List the specific build host operating systems, CPUs, and compilers used.
    • Specify the target architectures, boards, and configurations used for testing.
    • Include relevant snippets of testing logs before and after the changes to demonstrate the impact of the modifications.

By addressing these points, you can significantly improve the clarity and completeness of your PR, making it easier for reviewers to understand and approve your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants