From owner-svn-src-all@freebsd.org Mon Nov 19 15:20:08 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 147DF110655B; Mon, 19 Nov 2018 15:20:08 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id AA7A179738; Mon, 19 Nov 2018 15:20:06 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id E7E2810562FF; Tue, 20 Nov 2018 02:19:57 +1100 (AEDT) Date: Tue, 20 Nov 2018 02:19:57 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans cc: Warner Losh , Andriy Gapon , "Rodney W. Grimes" , Allan Jude , Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r340450 - head/sys/sys In-Reply-To: <20181120005944.B1050@besplex.bde.org> Message-ID: <20181120014951.J1250@besplex.bde.org> References: <201811190104.wAJ14CaE059062@pdx.rh.CN85.dnsmgr.net> <5e227743-6463-d395-f2ba-da8d4ba248ca@FreeBSD.org> <20181120005944.B1050@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=P6RKvmIu c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=04AClt5CvOlRR2CIbe8A:9 a=ODUrRZQBT3x5M27z:21 a=yO6B2NnfoD2nWY7K:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-Rspamd-Queue-Id: AA7A179738 X-Spamd-Result: default: False [-0.86 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-0.83)[-0.829,0]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; FREEMAIL_FROM(0.00)[optusnet.com.au]; R_SPF_ALLOW(-0.20)[+ip4:211.29.132.0/23]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[optusnet.com.au]; NEURAL_SPAM_SHORT(0.28)[0.284,0]; TO_MATCH_ENVRCPT_SOME(0.00)[]; MX_GOOD(-0.01)[cached: extmail.optusnet.com.au]; RCPT_COUNT_SEVEN(0.00)[9]; IP_SCORE(-0.01)[country: AU(-0.04)]; FREEMAIL_TO(0.00)[optusnet.com.au]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[optusnet.com.au]; ASN(0.00)[asn:4804, ipnet:211.28.0.0/14, country:AU]; RCVD_COUNT_TWO(0.00)[2]; RCVD_IN_DNSWL_LOW(-0.10)[249.132.29.211.list.dnswl.org : 127.0.5.1] X-Rspamd-Server: mx1.freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Nov 2018 15:20:08 -0000 On Tue, 20 Nov 2018, Bruce Evans wrote: > On Mon, 19 Nov 2018, Warner Losh wrote: > >> On Mon, Nov 19, 2018 at 12:31 AM Andriy Gapon wrote: > >>> 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. > > I checked them on all valid values. They obviously can't work, since the > constant is garbage. Unfortunately, I lost the test program. IIRC, it > finds about 80 million out of 1 billion wrong results for nsec precision, > 32 out of 1 million wrong for usec precision, and 0 out of 1000 wrong for > msec precision. I found my test program. >> 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 > > This value is just invalid. Negative values are obviously invalid. Positive > values of more than 1 second are invalid. In the old version, values of > precisely 1 second don't overflow since the scale factor is rounded down; > the result is just 1 unit lower than 1 second. Overflow (actually > truncation) > occurs at 1 unit above 1 second. E.g., sbttoms(mstosbt(1000)) was 999 and > sbttoms(mstosbt(1001)) was 0. Now in my fixed version, > sbttoms(mstosbt(1000)) > is truncated to 0 and sbttoms(mstosbt(1001)) is truncated to 1. > > The test program also showed that in the old version, all valid args except > 0 are reduced by precisely 1 by conversion to sbt and back. > >> 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. > > Bug in the mlx code. The seconds part must be converted separately, as in > tstosbt(). mlx doesn't seem to use sbt directly, or even msleep(), so the bug does seem to be central. mlx actually uses mtx_sleep() with timo = hz(). mtx_sleep() is actually an obfuscated macro wrapping _sleep(). The conversion to sbt is done by the macro and is sloppy. It multiplies timo by tick_sbt. tick_sbt is SBT_1S / hz, so the sbt is SBT_1S / hz * hz which is SBT_1S reduced a little. This is not affected by the new code, and it still isn't converted back to 1 second in ms, us or ns. Even if it is converted back and then forth to sbt using the new code, it remains less than 1 second as an sbt so shouldn't cause any new problems. Here is the test program: XX /* Test decimal<->sbt conversions. */ XX XX #include XX XX #include XX XX int XX main(int argc, char **argcv) XX { XX uint64_t sbt; XX int i, j, wrong; XX XX wrong = 0; XX for (i = 0; i < 1000; i++) { XX sbt = mstosbt(i); XX j = sbttoms(sbt); XX if (i != j) { XX wrong++; XX if (argc > 1) XX printf("%d -> %#jx -> %d\n", XX i, (uintmax_t)sbt, j); XX } XX } XX printf("%d wrong values for ms\n", wrong); XX XX wrong = 0; XX for (i = 0; i < 1000000; i++) { XX sbt = ustosbt(i); XX j = sbttous(sbt); XX if (i != j) { XX wrong++; XX if (argc > 1) XX printf("%d -> %#jx -> %d\n", XX i, (uintmax_t)sbt, j); XX } XX } XX printf("%d wrong values for us\n", wrong); XX XX wrong = 0; XX for (i = 0; i < 1000000000; i++) { XX sbt = nstosbt(i); XX j = sbttons(sbt); XX if (i != j) { XX wrong++; XX if (argc > 1) XX printf("%d -> %#jx -> %d\n", XX i, (uintmax_t)sbt, j); XX } XX } XX printf("%d wrong values for ns\n", wrong); XX } Output: 0 wrong values for ms 32 wrong values for us 82602428 wrong values for ns Bruce