Skip to content

sched/hrtimer: Fix some functional correctness issues. #17784

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.

Sign up for GitHub

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

Jump to bottom
Merged
xiaoxiang781216 merged 5 commits into from
Jan 8, 2026
Merged

sched/hrtimer: Fix some functional correctness issues. #17784

xiaoxiang781216 merged 5 commits into from
Jan 8, 2026
+10 −6

Conversation

@Fix-Point
Copy link
Contributor

@Fix-Point Fix-Point commented Jan 7, 2026 •
edited by GUIDINGLI
Loading

Summary

This PR is very small part of the #17675.
This PR fixed some minor functional correctness errors and small improvements, including:

  1. Fixed missing Make.defs.
  2. Simplified the hrtimer pending state and decoupled with the rb-tree implementation (Reduce at least one branch with no performance degradation, but require modifying the API).
  3. Cause the HRtimer final purpose is replace the wdog with hrtimer, so add a func param for hrtimer_start() like wd_start()
  4. Renamed the callback type to hrtimer_entry_t, aligning with the wdentry_t in wdog.
  5. Added dependency on CONFIG_SYSTEM_TIME64 for the CONFIG_HRTIMER, since the hrtimer use clock_compare to compare 64-bit time. If we enabled CONFIG_HRTIMER without CONFIG_SYSTEM_TIME64, It will be totally functional incorrect.

In next patches, we will refactor the entire state machine to ensure the functional correctness in SMP and update the documentations.

Impact

HRTimer is not enabled yet, so it has no impact on systems.
This PR fixed some functional correctness issue and reduced the conditional branches.

Testing

Tested on rv-virt:smp, ostest passed.

@Fix-Point
sched/hrtimer: Add const for hrtimer argument.
d23af23 
The callback function can not modify the hrtimer.

Signed-off-by: ouyangxiangzhen <[email protected]>
@Fix-Point
sched/hrtimer: Fix hrtimer compilation.
0ae1ae8 
This commit fixed hrtimer compilation issue.

Signed-off-by: ouyangxiangzhen <[email protected]>
@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 Jan 7, 2026
@Fix-Point Fix-Point force-pushed the hrtimer_refactor_p1 branch 2 times, most recently from cc411cc to c7e12a5 Compare January 7, 2026 03:53
xiaoxiang781216
xiaoxiang781216 reviewed Jan 7, 2026
View reviewed changes
xiaoxiang781216
xiaoxiang781216 reviewed Jan 7, 2026
View reviewed changes
wangchdo
wangchdo reviewed Jan 7, 2026
View reviewed changes
wangchdo
wangchdo reviewed Jan 7, 2026
View reviewed changes
@wangchdo
Copy link
Contributor

wangchdo commented Jan 7, 2026

hrtimer ostest is already merged, please add log for hrtimer ostest when you update hrtimer module.

You can enable hrtimer with #17573

@xiaoxiang781216 xiaoxiang781216 mentioned this pull request Jan 7, 2026
Open
@Fix-Point Fix-Point force-pushed the hrtimer_refactor_p1 branch 7 times, most recently from 8c6f6bb to d3bf96a Compare January 7, 2026 07:29
GUIDINGLI
GUIDINGLI previously approved these changes Jan 7, 2026
View reviewed changes
wangchdo
wangchdo reviewed Jan 7, 2026
View reviewed changes
jerpelea
jerpelea previously approved these changes Jan 7, 2026
View reviewed changes
@wangchdo
Copy link
Contributor

wangchdo commented Jan 7, 2026 •
edited
Loading

@GUIDINGLI @Fix-Point

You guys need to provide sufficient reasoning before changing external APIs of existing module, I don't think just mark my comments as resolved without any replying is enough.

cc @xiaoxiang781216 @acassis @cederom

@GUIDINGLI
Copy link
Contributor

GUIDINGLI commented Jan 7, 2026

@GUIDINGLI @Fix-Point

You guys need to provide sufficient reasoning before changing external APIs of existing module, I don't think just mark my comments as resolved without any replying is enough.

cc @xiaoxiang781216 @acassis @cederom

The PR description has said why.

@Fix-Point
Copy link
Contributor Author

Fix-Point commented Jan 7, 2026 •
edited
Loading

@GUIDINGLI @Fix-Point

You guys need to provide sufficient reasoning before changing external APIs of existing module, I don't think just mark my comments as resolved without any replying is enough.

cc @xiaoxiang781216 @acassis @cederom

Now that your code has been merged, although this is not fair to me at all. I respect your effort and will fix the functional correctness issues based on your work. I have already wasted a lot of time, so please don't waste any more of our time, since we both have many other works to do, thank you.

@wangchdo
Copy link
Contributor

wangchdo commented Jan 7, 2026 •
edited
Loading

@GUIDINGLI @Fix-Point
You guys need to provide sufficient reasoning before changing external APIs of existing module, I don't think just mark my comments as resolved without any replying is enough.
cc @xiaoxiang781216 @acassis @cederom

The PR description has said why.

I just don't get it, why do we need hrtimer_start_absolute(hrtimer, period_func, UINT64_MAX)? Why do we need to make hrtimer align with wdog...

@wangchdo
Copy link
Contributor

wangchdo commented Jan 7, 2026 •
edited
Loading

@GUIDINGLI @Fix-Point
You guys need to provide sufficient reasoning before changing external APIs of existing module, I don't think just mark my comments as resolved without any replying is enough.
cc @xiaoxiang781216 @acassis @cederom

Now that your code has been merged, although this is not fair to me at all. I respect your effort and will fix the functional correctness issues based on your work. I have already wasted a lot of time, so please don't waste any more of our time, since we both have many other works to do, thank you.

Your response makes me feel sorry for NuttX community...

@GUIDINGLI
Copy link
Contributor

GUIDINGLI commented Jan 7, 2026

@GUIDINGLI @Fix-Point
You guys need to provide sufficient reasoning before changing external APIs of existing module, I don't think just mark my comments as resolved without any replying is enough.
cc @xiaoxiang781216 @acassis @cederom

The PR description has said why.

I just don't get it, why do we need hrtimer_start_absolute(hrtimer, period_func, UINT64_MAX)? Why do we need to make hrtimer align with wdog...

don‘t get your point, I haven't seen the hrtimer_start_absolute() API in this PR

xiaoxiang781216
xiaoxiang781216 reviewed Jan 7, 2026
View reviewed changes
@Fix-Point Fix-Point dismissed stale reviews from jerpelea and GUIDINGLI via 9c05ef4 January 7, 2026 15:04
@Fix-Point Fix-Point force-pushed the hrtimer_refactor_p1 branch 2 times, most recently from 9c05ef4 to a2a3f8c Compare January 7, 2026 15:59
@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 7, 2026 •
edited
Loading

@Fix-Point please drop the patch which pass func to hrtimer_start from this pr since it need more discussion to accept the change.

jerpelea
jerpelea previously approved these changes Jan 8, 2026
View reviewed changes
@Fix-Point
sched/hrtimer: Fix functional correctness issue.
6a723b2 
The expired field can not be used to indicate the cancelled state. Because
the `UINT64_MAX` is valid expired time. A periodic hrtimer started with
the hrtimer_start_absolute(hrtimer, period_func, UINT64_MAX) can
not be cancelled via `hrtimer_cancel`.

Signed-off-by: ouyangxiangzhen <[email protected]>
@Fix-Point
sched/hrtimer: Rename the callback type to hrtimer_entry_t.
37166db 
This commit renamed the callback type to `hrtimer_entry_t`, aligning with
the `wdentry_t` in wdog..

Signed-off-by: ouyangxiangzhen <[email protected]>
@Fix-Point
sched/hrtimer: Fix functional correctness issue.
6954874 
This commit added dependency on CONFIG_SYSTEM_TIME64 for the hrtimer, sin
ce the hrtimer use `clock_compare` to compare 64-bit time.

Signed-off-by: ouyangxiangzhen <[email protected]>
@Fix-Point Fix-Point force-pushed the hrtimer_refactor_p1 branch from a2a3f8c to 6954874 Compare January 8, 2026 11:24
@xiaoxiang781216 xiaoxiang781216 merged commit 254df17 into apache:master Jan 8, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acassis acassis acassis approved these changes

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

@GUIDINGLI GUIDINGLI GUIDINGLI approved these changes

@jerpelea jerpelea jerpelea left review comments

@yamt yamt Awaiting requested review from yamt yamt is a code owner

@masayuki2009 masayuki2009 Awaiting requested review from masayuki2009 masayuki2009 is a code owner

+1 more reviewer

@wangchdo wangchdo wangchdo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: OS Components OS Components issues Size: S The size of the change in this PR is small

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Fix-Point @wangchdo @GUIDINGLI @xiaoxiang781216 @acassis @jerpelea