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

Avoid wait_ms() spin #5216

Merged
merged 2 commits into from Oct 13, 2017

Conversation

Projects
None yet
7 participants
@kjbracey-arm
Contributor

kjbracey-arm commented Sep 28, 2017

System's wait_ms() spins to achieve a precise delay - we don't want this.
Call Thread::wait directly.

This PR is partly for testing, and to initiate discussion. I would personally like to see wait_ms() changed, or an alternative call in mbed_wait_api() brought in to avoid the unwanted CPU-sapping spin.

Avoid wait_ms() spin
System's wait_ms() spins to achieve a precise delay - we don't want this.
Call Thread::wait directly.
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 28, 2017

FAO @artokin

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 28, 2017

This PR is partly for testing, and to initiate discussion. I would personally like to see wait_ms() changed, or an alternative call in mbed_wait_api() brought in to avoid the unwanted CPU-sapping spin.

See please and chime in there: #5194 (comment) . We shall create an issue for this?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 28, 2017

Looks like you're thinking along the same lines as me, but for different reasons. Yes, I guess this should be an issue.

I agree with your proposal in that comment - wait_ms should be millisecond-precision and thread-sleepy, while wait_us is microsecond-precision and spinny. (Could conceivably be non-spinny using mbed_ticker_api?).

Not sure what to do with wait() - threshold based on value being less than 0.001? I've seen people doing wait(10e-6).

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 28, 2017

The reason for this change is that a blocking UARTSerial operation wants to poll quite frequently - here every millisecond, but because of the wait_ms(1) implementation, we'll end up spinning half the time on average. And worse, we could settle in to mostly-spinning or mostly-sleeping for prolonged periods, leading to noticeably different behaviour depending on which phase of the clock you first call the block.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 28, 2017

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 28, 2017

(And yes, obviously it should be using a proper wake-up mechanism rather than once-a-millisecond polling, but the first draft at such a mechanism got tangled up in review, and the ultimate solution which also handles ::poll requires Russ's condition variables or something like them. This is an interim improvement to the current interim functionality.)

@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 28, 2017

I agree having two different wait behaviors and 4 different wait calls is a bit confusing. Especially that wait() and Thread::wait() are different.

We can't do much about existing API right now, because of compatibility issues. But I've noticed that:

/** Waits a number of milliseconds.
 *
 *  @param ms the whole number of milliseconds to wait
 */
void wait_ms(int ms);

Is implemented incorrectly as

void wait_ms(int ms) {
    wait_us(ms * 1000);
}

The API doc says Waits a number of milliseconds. and it's implemented with possible sub-ms wait. We could just change the implementation to only include ms wait I would say.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 28, 2017

Making it just call Thread::wait(ms) is what I would expect and want, but it is an effective API change and difference from the non-RTOS one and current RTOS one.

wait_ms(1) for the current implementations will wait between 0.999ms and 1.000ms. It's guaranteeing both precision, and not undershooting (by more than a microsecond)
Thread::wait_ms(1) would wait between 0.000 and 1.000ms, depending on current tick phase.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 28, 2017

Agreed, implementing wait_ms in terms of system tick, right now, would change the behaviour quite a bit and break the compatibility.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 28, 2017

you could avoid breaking some of the backwards compatibility by delaying for an extra ms -
Thread::wait(ms + 1). That way wait_ms(1) would be guaranteed to wait at least 1.00ms and worst case 2.00ms. The worst case is no worse than a thread of equal priority stealing a tick due to round-robin scheduling.

As for this PR, the change looks good to me. That way UARTSerial won't be blocked on the wait_ms discussion.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 29, 2017

Unfortunately adding one could cause significant problems the other way - people trying to construct a 10ms periodic thing with a 10ms wait would get significant periodic drift (they'd have some anyway, but this would add a lot). Expecting them to specify "9" to get a 10ms period would be counterintuitive.

See this discussion here for people tearing their hair out over a built-in add 1 to RTX's osDelay that was quickly reverted. This scenario isn't quite the same, but I think the discussion is relevant. http://www.keil.com/forum/60393/

Not adding one is probably what the majority of users want - they'd get the period they expect with repeated calls. But there will be people wanting a minimum delay for some HW tolerance requirement...

I'd say "change to ticks + 1" is my least favourite of the 3 options for wait_ms. Haven't got a strong preference on the other two though.

  • leave as-is (deprecate?), introduce new documented-as-potentially-tick-based API
  • change to wait ticks
  • change to wait ticks + 1
@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 29, 2017

I don't think we can do much right now, except maybe implementing the functions in terms of the tickers. Going forward making both wait APIs work in the same way when they have the same name and having separate higher res (spining) wait function would make sense.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 2, 2017

Going back to the subject of the PR, there's also poll(), which like the block code in UARTSerial is indeed polling due to the lack of a wake-up mechanism.

And it really is spinning with just a yield - to make it perform the same as the block, I'll change that to a 1 millisecond wait too.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 2, 2017

Going back to the subject of the PR

So far @c1728p9 reviewed, and me (LGTM). Anyone else ? We are having jenkins CI issues currently that will be fixed ,same for circle CI. Then we can trigger morph build.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 2, 2017

@kjbracey-arm Could you change the sha of the last commit to kick out circle ci?

Make poll() use wait(1) rather than yield()
Spinning while polling is overly CPU intensive, and inconsistent with
the current blocking behaviour of UARTSerial.

Change to use Thread::wait(1) to match UARTSerial.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:UARTSerial_wait branch to de4ced3 Oct 3, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 9, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 10, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1554

All builds and test passed!

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 11, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

Build : SUCCESS

Build number : 103
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5216/

Triggering tests

/test mbed-os

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 removed the needs: CI label Oct 13, 2017

@theotherjimmy theotherjimmy merged commit 493e378 into ARMmbed:master Oct 13, 2017

6 checks passed

Cam-CI uvisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment