Date: Mon, 19 Nov 2018 00:53:27 -0700 From: Warner Losh <imp@bsdimp.com> To: Andriy Gapon <avg@freebsd.org> Cc: "Rodney W. Grimes" <rgrimes@freebsd.org>, Allan Jude <allanjude@freebsd.org>, Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r340450 - head/sys/sys Message-ID: <CANCZdfo4=TknaHR0%2B3fTCDzKO_EHF0Vfmkooi92ZaGEnVfq2bA@mail.gmail.com> In-Reply-To: <5e227743-6463-d395-f2ba-da8d4ba248ca@FreeBSD.org> References: <CANCZdfqr=R-MCpDEGWDqGYJbcQ46Hqw7PMrVinAsYTRPjLjJPA@mail.gmail.com> <201811190104.wAJ14CaE059062@pdx.rh.CN85.dnsmgr.net> <CANCZdfoGKKcL70ESKow=hfNABpO=5%2BQtUYcNmpt4gReRkeUvrA@mail.gmail.com> <5e227743-6463-d395-f2ba-da8d4ba248ca@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Nov 19, 2018 at 12:31 AM Andriy Gapon <avg@freebsd.org> wrote:
> On 19/11/2018 03:38, Warner Losh wrote:
> > I'll talk to Allan to see if he can test that. the bare 1 should be
> handled
> > properly because of C's promotion rules. 1ull << 32 is an unsigned long
> long.
> > What I really wanted was "~(uint32_t)0" but that construct has bit me in
> the past.
>
> I think that you could just do (unsigned int)-1 or UINT_MAX.
>
Perhaps.
> As a side note, I wonder if those functions are ever used on negative
> values,
> given the type of the argument, and if anyone checked their correctness in
> that
> case.
>
I don't think so, but an assert would be good to make sure.
But I think I understand the problem now.
mstosbt(1000) is overflowing with my change, but not without because we're
adding 2^32 to a number that's ~900 away from overflowing changing a very
large sleep of 1 second to a tiny sleep of 0 (which is 1ms at the default
Hz). I think we get this in the mlx code because there's a msleep(1000) in
at least one place. msleep(1001) would have failed before my change. Now I
think msleep(999) works, but msleep(1000) fails. Since the code is waiting
a second for hardware to initialize, a 1ms instead is likely to catch the
hardware before it's finished. I think this will cause problems no matter
cold or not, since it's all pause_sbt() under the covers and a delay of 0
is still 0 either way.
The fix I think is something like:
diff --git a/sys/sys/time.h b/sys/sys/time.h
index 2207767813f2..00c6bb4a20fb 100644
--- a/sys/sys/time.h
+++ b/sys/sys/time.h
@@ -204,8 +204,12 @@ sbttoms(sbintime_t _sbt)
static __inline sbintime_t
mstosbt(int64_t _ms)
{
+ sbintime_t sb;
- return ((_ms * (((uint64_t)1 << 63) / 500) + (1ull << 32) - 1) >>
32);
+ sb = (_ms / 1000) * SBT_1S;
+ _ms = _ms % 1000;
+ sb += (_ms * (((uint64_t)1 << 42) / 1000) + 1023) >> 10;
+ return (sb);
}
/*-
so we add the whole number of seconds first, then with the remainder we do
the math so we have enough precision to represent 1000 correctly. This lets
us then do the math with plenty of bits to spare. I need to reevaluate this
in the morning when I'm not so tired. It might be safe to not do the whole
seconds first, and I might need 11 bits instead of 10 to be safe, so I'll
hold off committing until I can probe the edge cases more thoroughly. I
thought long and hard about the tiny part of the edge case and never
stopped to think about what 1000ms would do...
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfo4=TknaHR0%2B3fTCDzKO_EHF0Vfmkooi92ZaGEnVfq2bA>