linux-kernel - Re: [PATCH v3] setlocalversion: work around "git describe" performance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK7LNATvMOR3qcp+-6koxnKkvMDRVHYJ0E0hyUaWLeV3-c0eBA@mail.gmail.com>
Date: Tue, 19 Nov 2024 08:19:41 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jeff King <peff@...f.net>, Sean Christopherson <seanjc@...gle.com>, Josh Poimboeuf <jpoimboe@...nel.org>
Subject: Re: [PATCH v3] setlocalversion: work around "git describe" performance

On Mon, Nov 18, 2024 at 8:01 PM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> Contrary to expectations, passing a single candidate tag to "git
> describe" is slower than not passing any --match options.
>
>   $ time git describe --debug
>   ...
>   traversed 10619 commits
>   ...
>   v6.12-rc5-63-g0fc810ae3ae1
>
>   real    0m0.169s
>
>   $ time git describe --match=v6.12-rc5 --debug
>   ...
>   traversed 1310024 commits
>   v6.12-rc5-63-g0fc810ae3ae1
>
>   real    0m1.281s
>
> In fact, the --debug output shows that git traverses all or most of
> history. For some repositories and/or git versions, those 1.3s are
> actually 10-15 seconds.
>
> This has been acknowledged as a performance bug in git [1], and a fix
> is on its way [2]. However, no solution is yet in git.git, and even
> when one lands, it will take quite a while before it finds its way to
> a release and for $random_kernel_developer to pick that up.
>
> So rewrite the logic to use plumbing commands. For each of the
> candidate values of $tag, we ask: (1) is $tag even an annotated
> tag? (2) Is it eligible to describe HEAD, i.e. an ancestor of
> HEAD? (3) If so, how many commits are in $tag..HEAD?
>
> I have tested that this produces the same output as the current script
> for ~700 random commits between v6.9..v6.10. For those 700 commits,
> and in my git repo, the 'make -s kernelrelease' command is on average
> ~4 times faster with this patch applied (geometric mean of ratios).
>
> For the commit mentioned in Josh's original report [3], the
> time-consuming part of setlocalversion goes from
>
> $ time git describe --match=v6.12-rc5 c1e939a21eb1
> v6.12-rc5-44-gc1e939a21eb1
>
> real    0m1.210s
>
> to
>
> $ time git rev-list --count --left-right v6.12-rc5..c1e939a21eb1
> 0       44
>
> real    0m0.037s
>
> [1] https://lore.kernel.org/git/20241101113910.GA2301440@coredump.intra.peff.net/
> [2] https://lore.kernel.org/git/20241106192236.GC880133@coredump.intra.peff.net/
> [3] https://lore.kernel.org/lkml/309549cafdcfe50c4fceac3263220cc3d8b109b2.1730337435.git.jpoimboe@kernel.org/
>
> Reported-by: Sean Christopherson <seanjc@...gle.com>
> Closes: https://lore.kernel.org/lkml/ZPtlxmdIJXOe0sEy@google.com/
> Reported-by: Josh Poimboeuf <jpoimboe@...nel.org>
> Tested-by: Josh Poimboeuf <jpoimboe@...nel.org>
> Closes: https://lore.kernel.org/lkml/309549cafdcfe50c4fceac3263220cc3d8b109b2.1730337435.git.jpoimboe@kernel.org/
> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
> ---
> v3:
>
> - Update trailer tag list, per Masahiro.
> - Drop redundant quoutes around the word tag
> - Add a shellcheck disable directive
>
> Masahiro, I decided to keep the changes minimal, in particular not to
> change anything around the logic or the (unused) return values, in
> order not to invalidate Josh' T-b tag. I think it's more important for
> this to make it to 6.13-rc1 (if that is even still possible, given
> that the MW is already open).


This is not urgent because it has been broken more than one year.

Your "|| return 1" may not live long.
https://lore.kernel.org/linux-kbuild/20241118231534.1351938-1-masahiroy@kernel.org/T/#u


If you write try_tag() like you wrote, my patch can become even simpler.




> +
> +       # $2 is the number of commits in the range $tag..HEAD, possibly 0.
> +       count="$2"

count=$2

is enough because double-quotes are not required on the RHS
of an assignment.




> -       # If we are at the tagged commit, we ignore it because the version is
> -       # well-defined.
> -       if [ "${tag}" != "${desc}" ]; then
> +       # If we are at the tagged commit, we ignore it because the
> +       # version is well-defined. If none of the attempted tags exist
> +       # or were usable, $count is still empty.
> +       if [ -z "${count}" ] || [ "${count}" -gt 0 ]; then

Is this code equivalent to the following?

         if [ "${count}" != 0 ]; then




-- 
Best Regards
Masahiro Yamada