Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190620062246.2665-1-e5ten.arch@gmail.com> <20191029210250.17007-1-e5ten.arch@gmail.com>
 <aaec7b5b-3a58-f36e-74e6-5be0a6185c5b@ideasonboard.com>
In-Reply-To: <aaec7b5b-3a58-f36e-74e6-5be0a6185c5b@ideasonboard.com>
From:   Ethan Sommer <e5ten.arch@gmail.com>
Date:   Wed, 30 Oct 2019 12:26:49 -0400
Message-ID: <CAMEGPiq66grdRyioma7pdftFAFD7BmK0w788nBXhx1XHxopj=Q@mail.gmail.com>
Subject: Re: [PATCH v3] replace timeconst bc script with an sh script
To:     Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc:     "H . Peter Anvin" <hpa@zytor.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Federico Vaga <federico.vaga@vaga.pv.it>,
        "Chang S. Bae" <chang.seok.bae@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@suse.de>,
        Mark Rutland <mark.rutland@arm.com>,
        John Stultz <john.stultz@linaro.org>,
        Kees Cook <keescook@chromium.org>,
        Corey Minyard <cminyard@mvista.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-doc@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> Running shellcheck on kernel/time/timeconst.sh produces the following
> warnings:
>
> shellcheck kernel/time/timeconst.sh
>
> In kernel/time/timeconst.sh line 11:
>         local i=1 j=0
>         ^-- SC2039: In POSIX sh, 'local' is undefined.
>
>
> In kernel/time/timeconst.sh line 20:
>         local i="$1" j="$2" k
>         ^-- SC2039: In POSIX sh, 'local' is undefined.
>
>
> In kernel/time/timeconst.sh line 34:
>         local i
>         ^-- SC2039: In POSIX sh, 'local' is undefined.
>
>
> In kernel/time/timeconst.sh line 44:
>         local i=0 j
>         ^-- SC2039: In POSIX sh, 'local' is undefined.
>
>
>
> Will this cause an issue for people running posix shells?
> Which shells have you tested your script on ?
While local is not part of POSIX, it is widely supported, including by
dash, and is used by many shell scripts in the kernel's build including
link-vmlinux.sh, however I can easily make the script work without local
by using unique variable names in each function and resetting all of
them at the beginning of the functions if that is desired instead of
local.
>
> Furthermore, I fear this conversion may not be suitable at present, as
> it produces potentially different results for CONFIG_HZ < 100
>
> (There may be more diffs, but I haven't yet compared a larger search space)
>
> using a quick script I put together to compare the output of
> timeconst.sh and timeconst.bc for a given CONFIG_HZ:
>
>
>  https://gist.github.com/kbingham/76e8718df7b7dc97361405cc1801a160
>
>
> $ for i in `seq 0 300`; do ./check-timeconst.sh $i; done
>
> produces a diff on almost every value 2 - 243
>
>    http://paste.ubuntu.com/p/wNggrfFZXB/
>
> Or rather 137 faults to be exact:
>
>  for i in `seq 0 250`; \
>         do ./check-timeconst.sh $i; \
>  done | grep -- "--- BC" | wc -l
>
>
> I think that might be considered a blocker to this implementation, or
> those values and the impact should certainly be investigated thoroughly.
>
> I haven't looked into detail into the change of any of those values, so
> I can not ascertain which one is more correct (though I suspect it's
> likely to be bc that will have the 'more correct' value)
>
> I would fear doing this in shell just isn't going to maintain the
> correct precision, which is likely a strong reason why bc was selected
> in the first place.
>
>
> If you can find the issue that causes this diff in your shell
> processing, and clarify or fix it - then it might be possible to gain
> some backing to the implementation, but even then it might become a
> shell specific precision issue ...
That definitely is a blocker, I will attempt to make this implementation
match the bc script for those values.