linux-kernel - Re: [PATCH v7 4/4] ARM: Add support for Hisilicon Kunpeng L3 cache controller

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]

Date:   Tue, 2 Feb 2021 09:44:48 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Zhen Lei <thunder.leizhen@...wei.com>
Cc:     Russell King <rmk+kernel@....linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Will Deacon <will.deacon@....com>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Rob Herring <robh+dt@...nel.org>,
        Wei Xu <xuwei5@...ilicon.com>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 4/4] ARM: Add support for Hisilicon Kunpeng L3 cache controller

On Tue, Feb 2, 2021 at 8:16 AM Zhen Lei <thunder.leizhen@...wei.com> wrote:
> +
> +/*
> + * All read and write operations on L3 cache registers are protected by the
> + * spinlock, except for l3cache_init(). Each time the L3 cache operation is
> + * performed, all related information is filled into its registers. Therefore,
> + * there is no memory order problem when only _relaxed() functions are used.

Thank you for including the text.

I don't think the explanation with the spin_lock() explains why this
can be considered safe though, as spin_lock() only contains serialization
against other CPUs (smp_mb()) rather than the stronger DMA barriers
implied by readl and writel. As Russell previously explained, these
barriers are the L1 cache operations (e.g. v7_dma_inv_range) do
include stronger barriers, so it would be better to come up with a
justification based on those.

> + * This can help us achieve some performance improvement:
> + * 1) The readl_relaxed() is about 20ns faster than readl().
> + * 2) The writel_relaxed() is about 123ns faster than writel().

These are not really the performance numbers I asked for, as a
low-level benchmark comparing the instructions is rather meaningless.
The time spent waiting for the barrier depends on what else is going
on around the barrier. Also, most of the time would likely be
spent spinning in the loop around readl() while the cache operations
are in progress, so the latency of a single readl() is not necessarily
significant.

To have a more useful performance number, try mentioning the
most performance sensitive non-coherent DMA master on one
of the chips that has this cache controller, and a high-level
performance number such as "1.2% more network packets per
second" if that is something you can measure easily.

Of course, if all high-speed DMA masters on this chip are
cache coherent, there is no need for performance numbers, just
mention that we don't care about speed in that case.

        Arnd