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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Guo Hui <guohui@uniontech.com>, sboyd@kernel.org
Cc:     jstultz@google.com, wangxiaohua@uniontech.com,
        linux-kernel@vger.kernel.org, Guo Hui <guohui@uniontech.com>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] timekeeping:add padding in timekeeper for Unixbench pipe
In-Reply-To: <20230106062946.19983-1-guohui@uniontech.com>
References: <20230106062946.19983-1-guohui@uniontech.com>
Date:   Mon, 09 Jan 2023 16:29:32 +0100
Message-ID: <874jszlo9v.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Guo!

On Fri, Jan 06 2023 at 14:29, Guo Hui wrote:
> When the LLC cache line size is 128 bytes, such as Kunpeng 920,
> the seq attribute and xtime attribute in the structure tk_core
> are completely in the same LLC cache line,
> and xtime_sec is the data protected by the seq lock
> in the function ktime_get_coarse_real_ts64,
> so seq and xtime_sec are in the same LLC cache line
> causing the false sharing problem.

What exactly causes the alleged false sharing problem?

> Adding padding before xtime_sec in the structure timekeeper
> is based on the comment of the structure tk_read_base: "This
> struct has size 56 byte on 64 bit. Together with a seqcount
> it occupies a single 64byte cache line." Therefore,
> seq and the structure tk_read_base
> should be placed in the same 64-byte cacheline.

How is that relevant? They _are_ in the same cacheline independent of
your padding, no?
                                           Offset  Size
  seqcount_raw_spinlock_t seq;          /*     0     4 */
  struct timekeeper       timekeeper;   /*     8   280 */
    struct tk_read_base     tkr_mono;   /*     8    56 */

8 + 56 = 64 which is also the case with your padding.

If your false sharing thing exists then all other timekeeper read
functions which only use

     tk_core.seq and tk_core.timekeeper.tkr_mono

have the very same false sharing problem because seq and data are in the
same cache line, no?

Aside of that, for architectures with 64 byte cache line size, your
change is fundamentally bad. Why?

It moves xtime_sec to offset 128 which means seq and xtime_sec are not
longer in consecutive cache lines.

If you change core data structures for the benefit of your platform,
then you have to provide proof that it does not cause any issues on
other architectures and platforms.

Thanks,

        tglx