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

sched_processtimer: use small lock to protect g_timer_interval and g_timer_tick #15129

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

Conversation

hujun260
Copy link
Contributor

Summary

sched_processtimer: use small lock to protect g_timer_interval and g_timer_tick

reason:
We would like to replace the critical section with a small lock.

Impact

sched_processtimer

Testing

ci ostest

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Dec 11, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 11, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the what, it lacks sufficient detail on the why and how. The Impact and Testing sections are also severely lacking.

Here's a breakdown of what's missing:

  • Summary:

    • Missing "Why": Why is replacing the critical section with a small lock necessary? What problem does it solve? Performance improvement? Deadlock avoidance? Explain the rationale.
    • Missing "How": What specific changes were made to the code to implement the small lock? Which lock is used? How does this interact with other locking mechanisms?
  • Impact:

    • Insufficient Detail: Simply stating "sched_processtimer" is not enough. Address all the impact points. At minimum, explain if this impacts users, the build, hardware, documentation, security, or compatibility. Even if the answer is "NO" for each, explicitly state it. For example: "Impact on user: NO".
  • Testing:

    • Insufficient Detail: "ci ostest" is not sufficient. Provide specifics:
      • Build Host(s): What operating system, CPU architecture, and compiler version were used for building?
      • Target(s): What architecture and board/configuration were used for testing? Was this tested on a simulator or real hardware?
      • Logs: The provided log sections are empty. Include actual logs demonstrating the behavior before and after the change. Show how the change addresses the problem described in the "Why" section of the summary.

In short, the PR needs significantly more detail to be considered complete. It needs to clearly explain the motivation, implementation, and testing of the change.


/* Process the timer ticks and set up the next interval (or not) */

nexttime = nxsched_timer_process(ticks, elapsed, false);

flags = enter_critical_section();
flags = spin_lock_irqsave_wo_note(&g_lock);
g_timer_interval = nxsched_timer_start(ticks, nexttime);
Copy link
Contributor

Choose a reason for hiding this comment

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

nxsched_timer_start() will access the global variables in lower harlf driver, using local spinlock cannot guarantee the same effect as csection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nxsched_timer_start will call up_timer_start or up_alarm_start
I believe that these two functions should be responsible for adding their own critical sections. In fact, this is probably how it's done in most cases.
image
image

However, there may be some overlooked scenarios. Currently, we can add an additional critical section in this function. Once all the timer driver codes have been replaced with small locks in the future, this critical section can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

…timer_tick

reason:
We would like to replace the critical section with a small lock.

Signed-off-by: hujun5 <[email protected]>
@@ -388,6 +391,8 @@ static clock_t nxsched_timer_start(clock_t ticks, clock_t interval)
}
#endif

flags = enter_critical_section();
Copy link
Contributor

Choose a reason for hiding this comment

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

why mix the critical section and spinlock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we need to protect up_timer_tick_start.

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

Successfully merging this pull request may close these issues.

4 participants