From owner-svn-src-head@freebsd.org Mon Nov 19 20:57:22 2018 Return-Path: Delivered-To: svn-src-head@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 D5BBF11272CF for ; Mon, 19 Nov 2018 20:57:21 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-it1-x135.google.com (mail-it1-x135.google.com [IPv6:2607:f8b0:4864:20::135]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AB6538C1C8 for ; Mon, 19 Nov 2018 20:57:20 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-it1-x135.google.com with SMTP id p11-v6so8326340itf.0 for ; Mon, 19 Nov 2018 12:57:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=V9keR/S2XANhR74kkQO4MluzrMO8+5jqKQvG0LkGQqw=; b=H/hT19hS8h/zoljwRQHRKDISXei8iZzRQrjBZ8ROu+ADmjtVHv41ZovCxFAKXKnlYl 5y5xq/Mvn+3vD69jVKVqEzyMsWKP3E9eeqXXhEdFqoOcwtBgLjs04xDgRL4FRwT655iP 9Zp5fIKL0lMsfGA9jOn1A5/l7xQgBMoYQrRiGxVc77zk9jcQM8GEhw9PFbI8RwY95jZW PVfWOnBiUEm/yLTvYqQAghcsnvBrZKSGWviklgn8uQGaFd8m0HtzKeQOsUyg6EWtOhka knEn/K/b2aArfP1KgkfVs1how/ateaJq/5caEdWx+4YnQF6D2owPACon0NPWZh9yRWD+ opjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=V9keR/S2XANhR74kkQO4MluzrMO8+5jqKQvG0LkGQqw=; b=PbNLHy5LCflwn71Uz0wTiOD27IAanpAG+wvKSd12iaaJrBoQfUpQ54OyW0ss4UA6Hn 2jDVjmE7WxWMlrpgPNTIjlfoYnL+weuiF1P2Sp4ZXADVQd44Iy456ih1aOHhYcnquz8f 1MEIiJw1JGJNYL5CXudrzGX6iAwBiHWjCtQEn9v2mZ1TP9VJ9yUTqyvyFqFxwEsA+OUd lpf4nl/FM2XLIC6goOkUZhWRJpRRObGFj0QX68TccuWetFWuO3mwAhZ0BtV7JM9eL7bk NtoE0A2j3HKYCwpW88RpelnSC3yciZuzTrLkbtIaPrfZZDAhzhjCwzUUJsPzLH6+FKEC 33kA== X-Gm-Message-State: AA+aEWZ/qcicFOVe8RE3X47jYgJ2MjIm9xJLMmFPTW4Icb15UDHV5ZOE 2S2UPklvwTeyo/2BPFmLH6oAHzRP1diU/g+3+JU+yQ== X-Google-Smtp-Source: AJdET5fijaFkrDI9Eau8iZHa8cn/VTduwL2zrrEPt3cvZRuRFYyZnvEG5WL6R3hmbvG5ySTyX1o5+8iPeRThg8yPGa4= X-Received: by 2002:a24:5f94:: with SMTP id r142-v6mr10140749itb.171.1542661039677; Mon, 19 Nov 2018 12:57:19 -0800 (PST) MIME-Version: 1.0 References: <201811190104.wAJ14CaE059062@pdx.rh.CN85.dnsmgr.net> <5e227743-6463-d395-f2ba-da8d4ba248ca@FreeBSD.org> <20181120005944.B1050@besplex.bde.org> <20181120014951.J1250@besplex.bde.org> <20181120023823.M1428@besplex.bde.org> In-Reply-To: <20181120023823.M1428@besplex.bde.org> From: Warner Losh Date: Mon, 19 Nov 2018 13:57:08 -0700 Message-ID: Subject: Re: svn commit: r340450 - head/sys/sys To: Bruce Evans Cc: Andriy Gapon , "Rodney W. Grimes" , Allan Jude , Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: AB6538C1C8 X-Spamd-Result: default: False [-4.20 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_SHORT(-0.98)[-0.977,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-head@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; TO_MATCH_ENVRCPT_SOME(0.00)[]; MX_GOOD(-0.01)[cached: ALT1.aspmx.l.google.com]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; RCPT_COUNT_SEVEN(0.00)[8]; RCVD_IN_DNSWL_NONE(0.00)[5.3.1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; R_SPF_NA(0.00)[]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; FREEMAIL_TO(0.00)[optusnet.com.au]; RCVD_TLS_LAST(0.00)[]; IP_SCORE(-2.22)[ip: (-6.89), ipnet: 2607:f8b0::/32(-2.45), asn: 15169(-1.67), country: US(-0.09)]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Server: mx1.freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Nov 2018 20:57:22 -0000 On Mon, Nov 19, 2018 at 9:05 AM Bruce Evans wrote: > On Tue, 20 Nov 2018, Bruce Evans wrote: > > > 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: > > ... > > 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 are all uses of these functions in kern outside of sys/time.h: > > XX arm/arm/mpcore_timer.c: sc->et.et_min_period = nstosbt(20); > > OK since 20 ns is less than 1 second. > > XX cddl/compat/opensolaris/sys/kcondvar.h: return > (cv_timedwait_sbt(cvp, mp, nstosbt(tim), nstosbt(res), 0)); > XX cddl/contrib/opensolaris/uts/common/fs/zfs/dmu_tx.c: > pause_sbt("dmu_tx_delay", nstosbt(wakeup), > XX cddl/contrib/opensolaris/uts/common/fs/zfs/dmu_tx.c: > nstosbt(zfs_delay_resolution_ns), C_ABSOLUTE); > XX cddl/contrib/opensolaris/uts/common/fs/zfs/zil.c: sbintime_t sleep = > nstosbt((zilog->zl_last_lwb_latency * pct) / 100); > XX compat/linuxkpi/common/include/linux/delay.h: > pause_sbt("lnxsleep", mstosbt(ms), 0, C_HARDCLOCK); > XX compat/linuxkpi/common/src/linux_hrtimer.c: > nstosbt(hrtimer->expires), nstosbt(hrtimer->precision), 0); > XX compat/linuxkpi/common/src/linux_hrtimer.c: > callout_reset_sbt(&hrtimer->callout, nstosbt(ktime_to_ns(time)), > XX compat/linuxkpi/common/src/linux_hrtimer.c: nstosbt(nsec), > hrtimer_call_handler, hrtimer, 0); > XX compat/linuxkpi/common/src/linux_hrtimer.c: > callout_reset_sbt(&hrtimer->callout, nstosbt(ktime_to_ns(interval)), > XX compat/linuxkpi/common/src/linux_hrtimer.c: > nstosbt(hrtimer->precision), hrtimer_call_handler, hrtimer, 0); > XX compat/linuxkpi/common/src/linux_schedule.c: ret = > -pause_sbt("lnxsleep", mstosbt(ms), 0, C_HARDCLOCK | C_CATCH); > > All of the above might are broken unless their timeout arg is restricted to > less than 1 second. > > Also, at least the sleep and pause calls in the above probably have a > style bug in knowing about sbt's at all. More important functions > like msleep() hide the use of sbt's and use fuzzier scale factors like > tick_sbt. > Yes. It's for these users I'm fixing the >= 1s cases. > XX dev/iicbus/nxprtc.c: pause_sbt("nxpotp", mstosbt(100), > mstosbt(10), 0); > XX dev/iicbus/nxprtc.c: pause_sbt("nxpbat", mstosbt(100), 0, 0); > > OK since 100 ms is less than 1 second. > > XX kern/kern_sysctl.c: sb = ustosbt(tt); > XX kern/kern_sysctl.c: sb = mstosbt(tt); > > Recently added bugs. The args is supplied by applications so can be far > above > 1 second. > > Also, these functions have a bogus API that takes uint64_t args. sbt's > can't represent that high, and the API doesn't support failure, so callers > have the burden of passing valid values. It is easiest to only support > args > up to 1 second and require the caller to handle seconds. > It's just as easy to cope. > Lots of itimer and select() code handle the corresponding problem for > timevals and timespecs almost correctly. Timevals and timespecs already > have the seconds part separate and itimer and select() syscalls do validity > checking on the fractional seconds, so there is no problem converting the > fractional sections to an sbt. Howver, the seconds part has type time_t > and this can be int64_t, which sbt's cannot represent. Also, POSIX doesn't > permit failure for seconds values that are representable by time_t. The > almost correct handling is to split up large seconds values in some cases > and break POSIX conformance by silently reducing the seconds value in > other cases. The reduction is usually to INT32_MAX / 2. This allows > adding 2 limited values but it is not obvious that this is enough since > rounding up twice or just adding 2 more would give overflow. > Right, that's beyond the scope of what I'm fixing. > XX kern/subr_rtc.c: sbt = nstosbt(waitns); > > This is correct, since waitns is the nanoseconds part of a timespec. > Yup. https://reviews.freebsd.org/D18051 Contains the changes. They address all the actual problems you've raised, but I'm going to disagree that 'ull' is bogus. For FreeBSD, it's fine and it's a lot more readable than the alternatives. Warner