Page MenuHomeFreeBSD

Ensure that all values of ns, us and ms work for {n,u,m}stosbt
ClosedPublic

Authored by imp on Nov 19 2018, 8:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jul 10, 5:25 AM
Unknown Object (File)
Mon, Jul 8, 6:23 AM
Unknown Object (File)
Tue, Jul 2, 2:31 AM
Unknown Object (File)
Fri, Jun 28, 9:42 AM
Unknown Object (File)
Sun, Jun 23, 8:49 PM
Unknown Object (File)
Sun, Jun 16, 12:01 PM
Unknown Object (File)
May 22 2024, 3:36 AM
Unknown Object (File)
May 21 2024, 10:05 PM
Subscribers
None

Details

Summary

Integer overflows and wrong constants limited the accuracy of these
functions and created situatiosn where sbttoXs(Xstosbt(Y)) != Y. This
was especailly true in the ns case where we had millions of values
that were wrong.

Instead, used fixed constants because there's no way to say ceil(X)
for integer math. Document what these crazy constants are.

Also, use a shift one fewer left to avoid integer overflow causing
incorrect results, and adjust the equasion accordingly. Document this.

Allow times >= 1s to be well defined for these conversion functions
(at least the Xstosbt). There's too many users in the tree that they
work for >= 1s.

This fixes a failure on boot to program firmware on the mlx4
NIC. There was a msleep(1000) in the code. Prior to my recent rounding
changes, msleep(1000) worked, but msleep(1001) did not because the old
code rounded to just below 2^64 and the new code rounds to just above
it (overflowing, causing the msleep(1000) to really sleep 1ms).

A test program to test all cases will be committed shortly. The test
exaustively tries every value (thanks to bde for the test).

Sponsored by: Netflix, Inc

Test Plan

bde's test program.
With the current in-tree code, we get lots of errors
With these changes we get 0.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp added reviewers: cem, allanjude, ian.

LGTM modulo nits and assuming that test case will be committed shortly.

sys/sys/time.h
172 ↗(On Diff #50614)

Inconsistent space around >>

174 ↗(On Diff #50614)

Missing right paren

176 ↗(On Diff #50614)

"lesser" :)

180–181 ↗(On Diff #50614)

#ifdef _KERNEL?

201 ↗(On Diff #50614)

Any reason not to represent 0x7fffffff as an expression rather than additional magic number? (UINT32_MAX >> 1) or something? (Maybe that would require header pollution?)

And don't we need to add sb back in?

This revision is now accepted and ready to land.Nov 19 2018, 10:47 PM
imp marked 5 inline comments as done.Nov 19 2018, 11:00 PM

OK. I think that some of the other ways to spell stuff may be more obscure.

sys/sys/time.h
180–181 ↗(On Diff #50614)

I want the assert all the time, not just when it's in the kernel. It's an imperfect world.

201 ↗(On Diff #50614)

sb += is needed here.
and I think 0x7fffffff is more expressive than uint32_max because this could be spelled as 31-1's, ones(31), etc. Not worth the extra hair. It's really unrelated to uint32_t, but rather it's exactly 31 bits that's important. Also, the constant would require yet another polluting include.

imp edited the test plan for this revision. (Show Details)

cem's changes

This revision now requires review to proceed.Nov 19 2018, 11:01 PM
imp marked 2 inline comments as done.Nov 19 2018, 11:02 PM

LGTM!

sys/sys/time.h
180–181 ↗(On Diff #50614)

ifdef _KERNEL is enabled more of the time than commented out is.

201 ↗(On Diff #50614)

The pollution is a good enough reason for me. I disagree with the idea that uint32_max>>1 doesn't represent exactly 31 bits of 1s, but it's a moot point.

This revision is now accepted and ready to land.Nov 19 2018, 11:11 PM

Confirmed that this fixes my mlx4en(4) not attaching

KASSERT where possible...

This revision now requires review to proceed.Nov 20 2018, 2:40 AM
imp marked 2 inline comments as done.Nov 20 2018, 2:42 AM

updated with kasserts enabled as much as I can...

sys/sys/time.h
180–181 ↗(On Diff #50614)

I'll do #ifdef KASSERT ... It's more direct, but there's a possibility of namespace pollution causing grief.

Works for me, thanks. Could we use a similar trick for userspace asserts?

#ifdef assert
assert(foo >= bar);
#endif

That starts to get a bit repetitive, so perhaps something like the following:

#if defined(KASSERT)
#define MY_KASSERT(cond, prfargs) KASSERT(cond, prfargs)
#elif defined(assert)
#define MY_KASSERT(cond, prfargs) assert(cond)
#else
#define MY_KASSERT(cond, prfargs) /* nothing */
#endif

And then replace the #ifdef/KASSERT/#endif in each of these with MY_KASSERT().

And maybe #undef MY_KASSERT at the bottom to avoid pollution.

This revision is now accepted and ready to land.Nov 20 2018, 2:57 AM
This revision was automatically updated to reflect the committed changes.