From owner-svn-src-all@freebsd.org Mon Nov 19 20:57:22 2018
Return-Path: <owner-svn-src-all@freebsd.org>
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 D5C9511272D0
for <svn-src-all@mailman.ysv.freebsd.org>;
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 A92AF8C1C7
for <svn-src-all@freebsd.org>; Mon, 19 Nov 2018 20:57:20 +0000 (UTC)
(envelope-from wlosh@bsdimp.com)
Received: by mail-it1-x135.google.com with SMTP id m15so167720itl.4
for <svn-src-all@freebsd.org>; 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=tDRanNvbuhqPiEQX2SqDnr57b/HfGAJa1UvCOlLjWLeWx0ITVqwGsdNYbNpepQuaa8
VsxWOoAmK0WR7f5Y6hfad/cgnoD/VUm/8Q2iYup/OMIDap41P1GB4meNW+/xKcIw/dsL
cuxPXfRkBYKsqiVCFkjQF9ca1xFst3/ymvlVE5JgCRvJj6FdPQINvwPKFsT5bBS7oOkp
f0fWxwEYCKVfCuHcfgGWdCYIRL6qBQ+21feuh04IXOPbcCR1ItUQr2MH9RtDV53OGiVh
RxYBX6rzAJJc/DX7eicL68HoC1yVmg29CWGpVgxN+5anPkyrOfAUTRhkOxppegl7vror
dkZQ==
X-Gm-Message-State: AA+aEWbzpNkuuiFtXiLnKL/R1Lo75ucIQBgH4zXt9FTYHvucHv/oC/zw
Zmqkiu3HmFipn/Ux+Z2AJI1kEvcT+hCLK9ONwqlfGg==
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: <CANCZdfqr=R-MCpDEGWDqGYJbcQ46Hqw7PMrVinAsYTRPjLjJPA@mail.gmail.com>
<201811190104.wAJ14CaE059062@pdx.rh.CN85.dnsmgr.net>
<CANCZdfoGKKcL70ESKow=hfNABpO=5+QtUYcNmpt4gReRkeUvrA@mail.gmail.com>
<5e227743-6463-d395-f2ba-da8d4ba248ca@FreeBSD.org>
<CANCZdfo4=TknaHR0+3fTCDzKO_EHF0Vfmkooi92ZaGEnVfq2bA@mail.gmail.com>
<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 <imp@bsdimp.com>
Date: Mon, 19 Nov 2018 13:57:08 -0700
Message-ID: <CANCZdfp-e2UQ=xPt_kfCcqzWsYsUaaP+DYyQf4uy574-VuQ4kA@mail.gmail.com>
Subject: Re: svn commit: r340450 - head/sys/sys
To: Bruce Evans <brde@optusnet.com.au>
Cc: Andriy Gapon <avg@freebsd.org>, "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
X-Rspamd-Queue-Id: A92AF8C1C7
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-all@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-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" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Mon, 19 Nov 2018 20:57:22 -0000
On Mon, Nov 19, 2018 at 9:05 AM Bruce Evans <brde@optusnet.com.au> 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 <avg@freebsd.org> 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