drivers: timer : cortex_m_systick MAX_TICKS protection

[hakehuang]

when CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC set to 960M and CONFIG_SYS_CLOCK_TICKS_PER_SEC set to 100
the MAX_TICKS will be zero or even a negative value, which is not expected, at this time the COUNTER_MAX is
so need add a protection here

fixes: #36766

there used to be a workaround, not a fix,

either change the CONFIG_SYS_CLOCK_TICKS_PER_SEC=200 or
CONFIG_PM to set the CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC to 32678

[hakehuang]

test on mimxrt1170_evk without default sys_tick settings

*** Booting Zephyr OS build v4.0.0-2778-g1b70bfa14e28 ***
Running TESTSUITE tickless_concept
===================================================================
START - test_tickless_slice
elapsed slice 110, expected: <100, 110>
elapsed slice 100, expected: <100, 110>
elapsed slice 100, expected: <100, 110>
elapsed slice 100, expected: <100, 110>
 PASS - test_tickless_slice in 0.607 seconds
===================================================================
START - test_tickless_sysclock
time 640, 850
time 860, 1060
 PASS - test_tickless_sysclock in 0.426 seconds
===================================================================
TESTSUITE tickless_concept succeeded

------ TESTSUITE SUMMARY START ------

SUITE PASS - 100.00% [tickless_concept]: pass = 2, fail = 0, skip = 0, total = 2 duration = 1.033 seconds
 - PASS - [tickless_concept.test_tickless_slice] duration = 0.607 seconds
 - PASS - [tickless_concept.test_tickless_sysclock] duration = 0.426 seconds

------ TESTSUITE SUMMARY END ------

===================================================================
PROJECT EXECUTION SUCCESSFUL

[andyross]

This seems clumsy. Can't you add a BUILD_ASSERT() in 64 bit precision instead?

[hakehuang]

build_assert only assert during building, it does not resolve the problem, so if we want to CONFIG_SYS_CLOCK_TICKS_PER_SEC to be 100 and still running in that high frequency(960M). let me try to find a better way to implement this.

[gramsay0]

@hakehuang here's an old attempt at this that may be a useful reference: 5cbf8d8 #66601

[hakehuang]

@hakehuang here's an old attempt at this that may be a useful reference: 5cbf8d8 #66601

I guess the pr attempts to assert the setting in return we still need to increate the CONFIG_SYS_CLOCK_TICKS_PER_SEC to avoid this issue, and my attempt to still use the high frequency, but less tick, by downgrading the accuracy to its up-limits

[andyross]

Driving back by. Seems like the real bug here is that "- 1" subexpression that's rolling over when the quotient is zero. Why is that there? It's not mathematically correct. Is it to handle latency slop in the reset code maybe?

Nonetheless if you want the expression to be safe, it's IMHO more clearly expressed as something like this, which clamps the quotient before the subtraction to guarantee no rollover:

#define MAX_TICKS CLAMP(COUNTER_MAX / CYC_PER_TICK, 1, COUNTER_MAX) - 1

But your version seems fine too. I'm just concerned about that term and where it came from.

[andyross]

Heh, it was my doing, back in 2018 during the timer rewrites: see commit c535300

I think it's just been carried along voodoo-style ever since. It's harmless (except for the possibility of rollover at high clock rates), but I really don't think it's needed. Might be worth spinning a version that simply removes that subtraction and check that nothing breaks.

Basically I think the root cause here is a bug I wrote six years ago. :)

[andyross]

Also, just to be clear, the configuration being managed here is pathological: the clock rates is so high and the tick rate so low that it's not possible to deliver interrupts that slowly from the paltry 24 bit SysTick counter. Such a configuration basically has no control over timer interrupt frequency and would be much better served with a regular tick interrupt (which is what it's getting anyway) that avoids all the runtime complexity.

[hakehuang]

Also, just to be clear, the configuration being managed here is pathological: the clock rates is so high and the tick rate so low that it's not possible to deliver interrupts that slowly from the paltry 24 bit SysTick counter. Such a configuration basically has no control over timer interrupt frequency and would be much better served with a regular tick interrupt (which is what it's getting anyway) that avoids all the runtime complexity.

@andyross will you propose a consolidate solution? my fix is to use the maximum of the 24bit counter if the original setting exceed. I can close my PR, if so. Thanks for checking.

[andyross]

To be clear: I don't think there's anything wrong with this fix. It's just that (1) it's working around a rollover condition that shouldn't have been there to begin with, just to (2) handle a pathological case where "the timer can't even count as high as one tick".

I don't know the Right solution there, but I guess in my head it would look nicer as a BUILD_ASSERT() that produces a "Tickless does nothing here, use a ticked kernel" error or something.

[hakehuang]

(2) handle a pathological case where "the timer can't even count as high as one tick".

I don't know the Right solution there, but I guess in my head it would look nicer as a BUILD_ASSERT() that produces a "Tickless does nothing here, use a ticked kernel" error or something.

I add a message to show this, as warning will break CI.

[hakehuang]

@andyross please help to review again, thanks.

[andyross]

Looks great here.

[hakehuang]

@teburd please help to check thanks

[hakehuang]

@teburd please help to review

[golowanow]

@hakehuang - FYI #84051 (comment)

[hakehuang]

@hakehuang - FYI #84051 (comment)

does this pr fix your issue?

[hakehuang]

@teburd can you help to review this? thanks

[golowanow]

@hakehuang - FYI #84051 (comment)

does this pr fix your issue?

together with #84050 they resolve both overflows and timing instability like these:

Time Measurements for multiq sched queues
Timing results: Clock frequency: 25 MHz
REC: sched.add.thread.ReadyQ_tail.min         - Add threads of decreasing priority, min.           :     387 cycles ,   15480 ns :
REC: sched.add.thread.ReadyQ_tail.max         - Add threads of decreasing priority, max.           : 4278577 cycles , 171143080 ns :
REC: sched.add.thread.ReadyQ_tail.avg         - Add threads of decreasing priority, avg.           :  384082 cycles , 15363280 ns :
REC: sched.add.thread.ReadyQ_tail.stddev      - Add threads of decreasing priority, stddev.        : 1220073 cycles , 48802920 ns :
ReadyQ.add.to.tail.0000.waiters          - Add thread of priority (0)     : 4261800 cycles , 170472023 ns
ReadyQ.add.to.tail.0001.waiters          - Add thread of priority (0)     :     387 cycles ,    15486 ns
ReadyQ.add.to.tail.0002.waiters          - Add thread of priority (0)     :     387 cycles ,    15490 ns
ReadyQ.add.to.tail.0003.waiters          - Add thread of priority (0)     :     387 cycles ,    15490 ns
ReadyQ.add.to.tail.0004.waiters          - Add thread of priority (0)     : 4261800 cycles , 170472000 ns
ReadyQ.add.to.tail.0005.waiters          - Add thread of priority (0)     :     387 cycles ,    15486 ns
ReadyQ.add.to.tail.0006.waiters          - Add thread of priority (0)     :     387 cycles ,    15486 ns

[hakehuang]

@hakehuang - FYI #84051 (comment)

does this pr fix your issue?

together with #84050 they resolve both overflows and timing instability like these:

Time Measurements for multiq sched queues
Timing results: Clock frequency: 25 MHz
REC: sched.add.thread.ReadyQ_tail.min         - Add threads of decreasing priority, min.           :     387 cycles ,   15480 ns :
REC: sched.add.thread.ReadyQ_tail.max         - Add threads of decreasing priority, max.           : 4278577 cycles , 171143080 ns :
REC: sched.add.thread.ReadyQ_tail.avg         - Add threads of decreasing priority, avg.           :  384082 cycles , 15363280 ns :
REC: sched.add.thread.ReadyQ_tail.stddev      - Add threads of decreasing priority, stddev.        : 1220073 cycles , 48802920 ns :
ReadyQ.add.to.tail.0000.waiters          - Add thread of priority (0)     : 4261800 cycles , 170472023 ns
ReadyQ.add.to.tail.0001.waiters          - Add thread of priority (0)     :     387 cycles ,    15486 ns
ReadyQ.add.to.tail.0002.waiters          - Add thread of priority (0)     :     387 cycles ,    15490 ns
ReadyQ.add.to.tail.0003.waiters          - Add thread of priority (0)     :     387 cycles ,    15490 ns
ReadyQ.add.to.tail.0004.waiters          - Add thread of priority (0)     : 4261800 cycles , 170472000 ns
ReadyQ.add.to.tail.0005.waiters          - Add thread of priority (0)     :     387 cycles ,    15486 ns
ReadyQ.add.to.tail.0006.waiters          - Add thread of priority (0)     :     387 cycles ,    15486 ns

oh, that's a different issue, the max_tick can not be zero which is this PR to resolve

[hakehuang]

@golowanow can you help to review and kindly approve this? @teburd seems not interest this area anymore

[hakehuang]

@andyross , @golowanow please help to review and kindly approve. Thanks

[hakehuang]

@andyross can you help to review again. just a small change on 32 bit compatible. Thanks

[hakehuang]

@andyross please help to review

[hakehuang]

@andyross please help to check, no big change as last time. Thanks a lot.