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:	7 Feb 12:57
Reporter:	Kaiwang CHen (OCA)	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S5 (Performance)
Version:	8.0.21 and above	OS:	Any
Assigned to:		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.