Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 15 Jan 2023 16:29:04 +0000
From:   Conor Dooley <conor@kernel.org>
To:     Jisheng Zhang <jszhang@kernel.org>
Cc:     Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Anup Patel <anup@brainfault.org>,
        Atish Patra <atishp@atishpatra.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Conor Dooley <conor.dooley@microchip.com>,
        Andrew Jones <ajones@ventanamicro.com>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, kvm-riscv@lists.infradead.org
Subject: Re: [PATCH v4 06/13] riscv: introduce
 riscv_has_extension_[un]likely()
Message-ID: <Y8Qp0NcIPPGqUDGl@spud>
References: <20230115154953.831-1-jszhang@kernel.org>
 <20230115154953.831-7-jszhang@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="HUUCBRPY9dF6efL2"
Content-Disposition: inline
In-Reply-To: <20230115154953.831-7-jszhang@kernel.org>
Precedence: bulk


--HUUCBRPY9dF6efL2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hey Jisheng,

On Sun, Jan 15, 2023 at 11:49:46PM +0800, Jisheng Zhang wrote:
> Generally, riscv ISA extensions are fixed for any specific hardware
> platform, so a hart's features won't change after booting. This
> chacteristic makes it straightforward to use a static branch to check
> if a specific ISA extension is supported or not to optimize
> performance.
>=20
> However, some ISA extensions such as SVPBMT and ZICBOM are handled
> via. the alternative sequences.
>=20
> Basically, for ease of maintenance, we prefer to use static branches
> in C code, but recently, Samuel found that the static branch usage in
> cpu_relax() breaks building with CONFIG_CC_OPTIMIZE_FOR_SIZE[1]. As
> Samuel pointed out, "Having a static branch in cpu_relax() is
> problematic because that function is widely inlined, including in some
> quite complex functions like in the VDSO. A quick measurement shows
> this static branch is responsible by itself for around 40% of the jump
> table."
>=20
> Samuel's findings pointed out one of a few downsides of static branches
> usage in C code to handle ISA extensions detected at boot time:
> static branch's metadata in the __jump_table section, which is not
> discarded after ISA extensions are finalized, wastes some space.
>=20
> I want to try to solve the issue for all possible dynamic handling of
> ISA extensions at boot time. Inspired by Mark[2], this patch introduces
> riscv_has_extension_*() helpers, which work like static branches but
> are patched using alternatives, thus the metadata can be freed after
> patching.
>=20
> Link: https://lore.kernel.org/linux-riscv/20220922060958.44203-1-samuel@s=
holland.org/ [1]
> Link: https://lore.kernel.org/linux-arm-kernel/20220912162210.3626215-8-m=
ark.rutland@arm.com/ [2]
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

It'd be great, if, in the future, you would hold off on sending new
versions of patchsets where the previous version is still being
discussed [3].
~3 days between versions is not very much, especially when that includes
a weekend!
I know you replied there earlier today with your opinion, but please
give people a chance to read and respond, before resubmitting, so as not
to split discussion between several threads.

3 - https://lore.kernel.org/linux-riscv/2398293.3Lj2Plt8kZ@diego/

Thanks,
Conor.


--HUUCBRPY9dF6efL2
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCY8Qp0AAKCRB4tDGHoIJi
0pPnAQCsIsNUGq5IDZkIgbOznwtHyiQL3aKdAxLmntEOJDBOJgD/dvpGPPetV8gc
DnsHNxe/93fMhhgnj9EtBjFUkmIeug0=
=naIL
-----END PGP SIGNATURE-----

--HUUCBRPY9dF6efL2--