From owner-svn-src-head@freebsd.org Sat Oct 26 01:14:24 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 74C90161FB6 for ; Sat, 26 Oct 2019 01:14:24 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qt1-x841.google.com (mail-qt1-x841.google.com [IPv6:2607:f8b0:4864:20::841]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 470NL733xFz4H55 for ; Sat, 26 Oct 2019 01:14:23 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qt1-x841.google.com with SMTP id c21so6013439qtj.12 for ; Fri, 25 Oct 2019 18:14:23 -0700 (PDT) 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=iDTb7mnkdmlYk/CMcfOg6K8Z5nBnAroUgHhF+RPa50M=; b=z/faNAO/2PY/8+4iMKe3OPxa6HEys9b6ZlTtbv4jRt3zL4udlNd1StaI01LG/05Juf E7nULWN2uPKiuQP5HmFc5fGx+f/asBxX3l8FC6Ibqvf3KRCq0O8IKMnw7DmAXRodkQ7R gqs/qako3y1qGoGNUUs+jY/GwexG8nenjNK+QABuB8FBihJAsKqBFtuXh3JwXzoAfL+e uRV0qUnU2VIF1m2xmxU2a99MhSCxDJFdhsnfjxaS1HA/xRK55+OO0msCNt4Y2UNRvwfJ czkbyrrmjgQHS6qi7GT+9QMyvOxgHICL9VpXs5ji0VC/vqZzC0xwF32x4Ik7NRwuGbC2 LG7w== 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=iDTb7mnkdmlYk/CMcfOg6K8Z5nBnAroUgHhF+RPa50M=; b=Kqvl9QT7MVBcr1DFw91+DnX/fflALu/k9Gqg8e4FtmkRRppFFN2uTl4cFPJFcTWO3c ylt/ai6GImf+v687lDGF/89wj498Y/xniIvZh09E+QfS2uDrD2HjTrOdGy1qrcGCMojG LjuL02Zsntp5cGUe8JhccjPRIKSojmy+xP3wgX3C9ZAA7YMzWr17SiB7EUKrb1NzN/J0 8LOm5C0qgoUsHNgmEDM1MXsVqT4+RvuzVRuSxBvrmVRqpcRtOI4+ZgA0IBK4I+YYSOUw 8VnoUzeDT34VHvvFcnjd19GHFzRnp3Ac9ivb0XC4ONSSXlzB4a4ikYnFNNDQi9t5IeqO bPqQ== X-Gm-Message-State: APjAAAVKz3zqlN7jOLnrxD76P89RTmM1KX6MoSbcXOWXIFDh4707Hlau AjQ1tpqufVkBSbF84C9L8bLWRfugkL6YUfk9OvhcFw== X-Google-Smtp-Source: APXvYqxfSAqg+j5NMbGF7U9dVwq+/r5O/iPTvQF3F//BxJHL0u3yWSDKNoPdL9fZYjuNTP1o8aYtWOf20z2W5Xlewrs= X-Received: by 2002:a0c:ee49:: with SMTP id m9mr6439446qvs.118.1572052462146; Fri, 25 Oct 2019 18:14:22 -0700 (PDT) MIME-Version: 1.0 References: <8baac891351521c9e7148a0c0db9978645158dce.camel@freebsd.org> <201910260051.x9Q0pKl3058676@gndrsh.dnsmgr.net> In-Reply-To: <201910260051.x9Q0pKl3058676@gndrsh.dnsmgr.net> From: Warner Losh Date: Fri, 25 Oct 2019 19:14:10 -0600 Message-ID: Subject: Re: svn commit: r354076 - head/sys/dev/ow To: "Rodney W. Grimes" Cc: Ian Lepore , Andriy Gapon , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: 470NL733xFz4H55 X-Spamd-Bar: - Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=z/faNAO/; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::841) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-1.40 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-0.99)[-0.991,0]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-head@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; URI_COUNT_ODD(1.00)[3]; RCPT_COUNT_FIVE(0.00)[6]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; RCVD_IN_DNSWL_NONE(0.00)[1.4.8.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]; MIME_TRACE(0.00)[0:+,1:+,2:~]; IP_SCORE(-0.41)[ip: (2.48), ipnet: 2607:f8b0::/32(-2.40), asn: 15169(-2.05), country: US(-0.05)]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; RCVD_TLS_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2] 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: Sat, 26 Oct 2019 01:14:24 -0000 On Fri, Oct 25, 2019, 6:51 PM Rodney W. Grimes wrote: > > On Fri, 2019-10-25 at 19:03 +0300, Andriy Gapon wrote: > > > On 25/10/2019 18:56, Ian Lepore wrote: > > > > On Fri, 2019-10-25 at 18:51 +0300, Andriy Gapon wrote: > > > > > On 25/10/2019 18:46, Ian Lepore wrote: > > > > > > On Fri, 2019-10-25 at 15:38 +0000, Andriy Gapon wrote: > > > > > > > Author: avg > > > > > > > Date: Fri Oct 25 15:38:09 2019 > > > > > > > New Revision: 354076 > > > > > > > URL: https://svnweb.freebsd.org/changeset/base/354076 > > > > > > > > > > > > > > Log: > > > > > > > owc_gpiobus_read_data: compare times in sbintime_t units > > > > > > > > > > > > > > Previously the code used sbttous() before microseconds > > > > > > > comparison > > > > > > > in one > > > > > > > place, sbttons() and nanoseconds in another, division by > > > > > > > SBT_1US > > > > > > > and > > > > > > > microseconds in yet another. > > > > > > > > > > > > > > Now the code consistently uses multiplication by SBT_1US to > > > > > > > convert > > > > > > > microseconds to sbintime_t before comparing them with > > > > > > > periods > > > > > > > between > > > > > > > calls to sbinuptime(). This is fast, this is precise > > > > > > > enough > > > > > > > (below > > > > > > > 0.03%) and the periods defined by the protocol cannot > > > > > > > overflow. > > > > > > > > > > > > > > Reviewed by: imp (D22108) > > > > > > > MFC after: 2 weeks > > > > > > > > > > > > > > Modified: > > > > > > > head/sys/dev/ow/owc_gpiobus.c > > > > > > > > > > > > > > Modified: head/sys/dev/ow/owc_gpiobus.c > > > > > > > ============================================================= > > > > > > > ==== > > > > > > > ==== > > > > > > > ========= > > > > > > > --- head/sys/dev/ow/owc_gpiobus.c Fri Oct 25 15:02:50 > > > > > > > 2019 ( > > > > > > > r354 > > > > > > > 075) > > > > > > > +++ head/sys/dev/ow/owc_gpiobus.c Fri Oct 25 15:38:09 > > > > > > > 2019 ( > > > > > > > r354 > > > > > > > 076) > > > > > > > @@ -296,10 +296,10 @@ owc_gpiobus_read_data(device_t dev, > > > > > > > struct > > > > > > > ow_timing * > > > > > > > do { > > > > > > > now = sbinuptime(); > > > > > > > GETPIN(sc, &sample); > > > > > > > - } while (sbttous(now - then) < t->t_rdv + 2 && sample > > > > > > > == 0); > > > > > > > + } while (now - then < (t->t_rdv + 2) * SBT_1US && > > > > > > > sample == 0); > > > > > > > critical_exit(); > > > > > > > > > > > > > > - if (sbttons(now - then) < t->t_rdv * 1000) > > > > > > > + if (now - then < t->t_rdv * SBT_1US) > > > > > > > *bit = 1; > > > > > > > else > > > > > > > *bit = 0; > > > > > > > @@ -307,7 +307,7 @@ owc_gpiobus_read_data(device_t dev, > > > > > > > struct > > > > > > > ow_timing * > > > > > > > /* Wait out the rest of t_slot */ > > > > > > > do { > > > > > > > now = sbinuptime(); > > > > > > > - } while ((now - then) / SBT_1US < t->t_slot); > > > > > > > + } while (now - then < t->t_slot * SBT_1US); > > > > > > > > > > > > > > RELBUS(sc); > > > > > > > > > > > > > > > > > > > Unit conversions with sbt times should be done using the macros > > > > > > that > > > > > > carefully avoid roundoff errors. I don't understand why you've > > > > > > changed > > > > > > the code that correctly did use those macros to inline math. > > > > > > > > > > I think that the commit message explains it: > > > > > This is fast, this is precise enough (below 0.03%) and the > > > > > periods > > > > > defined by > > > > > the protocol cannot overflow. > > > > > > > > > > Do you disagree? > > > > > Could you please explain in which of the three lines changed the > > > > > new > > > > > code is > > > > > worse and why? > > > > > > > > > > > > > I absolutely disagree (or I wouldn't have replied). Unit > > > > conversions > > > > using sbt times should use the predefined macros, NOT incline > > > > multiplication and division. I don't know how to say it more > > > > clearly > > > > than that. The conversion macros are idiomatic (at least, they > > > > would > > > > be if people stopped writing inline conversion expressions). > > > > > > I can agree that I should have used ustosbt() instead of > > > multiplication by > > > SBT_1US, but I don't agree with your original message that I changed > > > the code > > > that correctly used the macros. > > > > > > But again, I know the times being converted, they are fixed by the > > > protocol and > > > I do not see why I have to use this: > > > > > > static __inline sbintime_t > > > ustosbt(int64_t _us) > > > { > > > sbintime_t sb = 0; > > > > > > #ifdef KASSERT > > > KASSERT(_us >= 0, ("Negative values illegal for ustosbt: > > > %jd", _us)); > > > #endif > > > if (_us >= SBT_1S) { > > > sb = (_us / 1000000) * SBT_1S; > > > _us = _us % 1000000; > > > } > > > /* 9223372036855 = ceil(2^63 / 1000000) */ > > > sb += ((_us * 9223372036855ull) + 0x7fffffff) >> 31; > > > return (sb); > > > } > > > > > > instead of this > > > x * (((sbintime_t)1 << 32) / 1000000) > > > > > > The times: > > > static struct ow_timing timing_regular = { > > > .t_slot = 60, /* 60 to 120 */ > > > .t_low0 = 60, /* really 60 to 120 */ > > > .t_low1 = 1, /* really 1 to 15 */ > > > .t_release = 45, /* <= 45us */ > > > .t_rec = 1, /* at least 1us */ > > > .t_rdv = 15, /* 15us */ > > > .t_rstl = 480, /* 480us or more */ > > > .t_rsth = 480, /* 480us or more */ > > > .t_pdl = 60, /* 60us to 240us */ > > > .t_pdh = 60, /* 15us to 60us */ > > > .t_lowr = 1, /* 1us */ > > > }; > > Should this chunk of code now have a bit fat NB: if you > modify these be careful, there is other code that assumes > the values here can not cause overflow? > No. The values won't com close to overflowing even at the slowest transfer speeds. The macros are most needed for nanosecond stuff, and these are all at most tens of microseconds... Warner > > > > > > > > > The fact that you could say all that means that there is nothing I can > > say that is going to change your mind. I will say if you ever do > > anything like this to code I wrote, I will revert it immediately. > > > > -- Ian > > > > > > -- > Rod Grimes > rgrimes@freebsd.org >