MySQL Bugs: #116878: False sharing with hardcoded cache line size

Bug #116878	False sharing with hardcoded cache line size
Submitted:	4 Dec 2024 17:10	Modified:	28 Jul 11:31
Reporter:	Kaiwang CHen (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S5 (Performance)
Version:	8.0.21 and above	OS:	Any
Assigned to:	Jakub Lopuszanski	CPU Architecture:	Any
Tags:	Contribution

Description:
Some data structures are not properly padded,

storage/innobase/include/read0types.h

class ReadView {
  // ...

  /** List of read views in trx_sys */
  byte pad1[64 - sizeof(node_t)];
  node_t m_view_list;
};

storage/ndb/src/kernel/vm/TransporterCallback.cpp

static struct ReceiverThreadCache {
  SectionSegmentPool::Cache cache_instance;
  char pad[64 - sizeof(SectionSegmentPool::Cache)];
} g_receiver_thread_cache[MAX_NDBMT_RECEIVE_THREADS];

They could be replaced by proper cache line sizes, as defined in
"WL#10314 - InnoDB: Lock-sys optimization: sharded lock_sys mutex" (8.0.21, 2020-07-13):

/** CPU cache line size */
#ifdef __powerpc__
constexpr size_t INNODB_CACHE_LINE_SIZE = 128;
#else
constexpr size_t INNODB_CACHE_LINE_SIZE = 64;
#endif /* __powerpc__ */

How to repeat:
Read the code.

Suggested fix:
I'm not sure the second because it belongs ndb. I think the first should be replaced with ut::INNODB_CACHE_LINE_SIZE.

See enclosed for a fix. (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: ReadView_pad1.diff (application/octet-stream, text), 463 bytes.

Hello Kaiwang CHen,

Thank you for the report and contribution.
Is this enhancement contribution targeting PowerPC architecture? Please confirm.

Please be informed that PowerPC is not a Supported Platform, EOL details are mentioned at https://www.mysql.com/support/eol-notice.html

regards,
Umesh

Hi, it's targeting arm arch. Another ifdef should be added, though.

Thanks for contribution!

I agree that hardcoding 64 is probably not the best idea, as for PowerPC it should be 128.
But, PowerPC itself isn't supported.

You mention ARM. May I know which specific platform?  We support arm64.

But many (most?) arm64 platforms use 64 bytes.
And we need a compile-time decision to use 64 or 128.
IIUC the situation with ARM64 is a bit complicated as in some cases like the one described in https://www.mono-project.com/news/2016/09/12/arm64-icache/ the same machine might have cache lines of two different lengths.

1. Can you share a patch proposal which actually solves your problem on arm, i.e. includes the required changes to INNODB_CACHE_LINE_SIZE definition? Or at least can you share your idea for how to do it correctly so that a single binary compiled on arm can be used on all arm machines?
2. Do you have a reproducible test scenario which shows the performance impact of such change?

I ask, because the current patch proposal doesn't seem to impact the resulting binary, except for PowerPC, which we don't support anyway (and thus should remove the `#ifdef __powerpc__` from the codebase).
And while I like the elegance of using INNODB_CACHE_LINE_SIZE instead of 64, I don't see how to justify it as a solution to any real problem.

No feedback was provided for this bug for over a month, so it is
being suspended automatically. If you are able to provide the
information that was originally requested, please do so and change
the status of the bug back to "Open".

Sorry, there's some mismatch between internal issue tracker's status and this one, and some bot got overeager. In fact the patch has been already merged to mysql-trunk, and is on its way to be released in 10.0.0 AFAICT.
Thank you for contribution

Posted by developer:
 
Added the following note to the MySQL Server 8.4.11, 9.7.2, and 26.7.0 release notes:

ReadView objects are shared through an intrusive list that is
frequently accessed by both transaction threads and the purge
thread. Their padding was intended to keep the list-link node on
a separate CPU cache line, avoiding cache contention between
these concurrent operations. However, the implementation assumed
a 64-byte cache line and calculated padding based on the list
node’s size, which did not reliably provide the intended
isolation. 
The padding now reserves a full, platform-specific
InnoDB cache line. This removes the hardcoded assumption and
ensures the list-link node is separated from other ReadView
fields regardless of its position within the object, reducing
the risk of false sharing on different hardware platforms.
Our thanks to Kaiwang Chen and the team at Tencent for the contribution.