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

trace: replace all *_wo_note to *_notrace to make api define more approachable #14943

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Nov 26, 2024

Summary

trace: replace all *_wo_note to *_notrace to make api define more approachable

Follow the linux naming style:

https://github.com/torvalds/linux/blob/master/include/linux/preempt.h#L237-L242
https://github.com/torvalds/linux/blob/master/include/linux/srcu.h#L221

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

Impact

N/A

Testing

ci-check

@github-actions github-actions bot added Area: Drivers Drivers issues Area: OS Components OS Components issues labels Nov 26, 2024
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Nov 26, 2024
@anchao anchao changed the title trace: replace all *_wo_note to _notrace to make api define more approachable trace: replace all *_wo_note to *_notrace to make api define more approachable Nov 26, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 26, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change and testing information, it lacks crucial details.

Here's a breakdown of what's missing:

  • Insufficient Summary: While the summary explains what was changed, it doesn't fully explain why. Why is the Linux naming style preferred? What problem does the current naming create? What benefit does the new naming provide? This is critical for reviewers to understand the motivation behind the change.

  • Impact Section is too brief: "N/A" is not acceptable. Even if there's minimal impact, each item should be addressed with a "NO" and a brief explanation justifying why there is no impact. For example, "Impact on user: NO - This is a purely internal naming change with no user-facing effects." This demonstrates that the impact has been considered. If the PR genuinely has no impact on any of these elements, that is rather uncommon. Changes to the naming of APIs are likely to have implications on documentation or backward compatibility as a bare minimum.

  • Missing Testing Details: "ci-check" is insufficient. While CI is important, the PR needs to specify what was tested and how. Provide specific test cases or commands used. "Before" and "After" logs are required, even if they are identical (to demonstrate no regression). If the CI covers the testing completely, then link to the specific CI run and highlight the relevant parts that demonstrate the change works. Also include details on the build host and target used for testing.

In short, the PR needs to be more thorough and explicit in addressing all the requirements, even if the impact or changes are minimal. The goal is to provide enough information for reviewers to quickly understand and assess the change.

@@ -1837,7 +1837,7 @@ void sched_note_filter_mode(FAR struct note_filter_named_mode_s *oldm,
irqstate_t irq_mask;
FAR struct note_driver_s **driver;

irq_mask = spin_lock_irqsave_wo_note(&g_note_lock);
irq_mask = spin_lock_irqsave_notrace(&g_note_lock);
Copy link
Contributor

Choose a reason for hiding this comment

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

but nuttx trace framework name note not trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But higher-level interfaces are trace models:
https://github.com/apache/nuttx/blob/master/include/nuttx/trace.h
could we rename trace framework from note to trace?

Copy link
Contributor

Choose a reason for hiding this comment

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

either way is fine, but we need be consistent.

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 think this may require 2 stages to finish this work:

  1. refine current code base, rename caller interface from note to trace.
  2. rename note driver module to trace

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I am fine with the proposal. please update the document too.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Dec 10, 2024

Choose a reason for hiding this comment

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

the patch doesn't fix the problem I rise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wo_ was added by Xiaomi. It is not in the history of nuttx. Some developers mistakenly believe that wo is the abbreviation of write only. The abbreviation wo(without) is not an English convention. I think we don't need to make any document changes.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Dec 10, 2024

Choose a reason for hiding this comment

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

Sorry, it isn't added by us but just follow the original code base. The first patch comes from:

commit ab3fa890239399de51e5482e595a751fee249b1c
Author: Masayuki Ishikawa <[email protected]>
Date:   Wed Jan 17 13:08:03 2018 +0900

    SMP: Introduce spin_lock_wo_note() and spin_unlock_wo_note()
    
    These APIs are used in sched_note.c to protect instumentation data.
    The deffrence between these APIs to exsiting spin_lock() and spin_unlock()
    is that they do not perform insturumentation to avoid recursive call
    when SCHED_INSTRUMENTATION_SPINLOCKS=y.

but xxnote is more consistent then xxtrace, since nuttx name the profile api as sched_note not trace. If you want to change the term, please change all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why would you allow trace.h to be merged into nuttx? And the kernel code is also using similar APIs?

https://github.com/apache/nuttx/blob/master/include/nuttx/trace.h
https://github.com/apache/nuttx/blob/master/sched/task/task_init.c#L97
image

should we rename all trace to note?

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Dec 10, 2024

Choose a reason for hiding this comment

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

Yes, @Gary-Hobson please fix the inconsistence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants