Come to think of it, the actual issue could be the lock_lsn() call in log_t::append_prepare(). Maybe we simply need a spin loop around that. Perhaps the existing implementation would work?

diff --git a/storage/innobase/include/log0log.h b/storage/innobase/include/log0log.h

index 54851ca0a65..1a6bde20cad 100644

--- a/storage/innobase/include/log0log.h

+++ b/storage/innobase/include/log0log.h

@@ -182,7 +182,7 @@ struct log_t

   typedef pthread_mutex_wrapper<true> log_lsn_lock;

 #else

   typedef srw_lock log_rwlock;

-  typedef srw_mutex log_lsn_lock;

+  typedef srw_spin_mutex log_lsn_lock;

 #endif

 public:

Another alternative would be the pthread_mutex_wrapper<true>, which we are using on ARM. That template parameter spinloop=true refers to MY_MUTEX_INIT_FAST a.k.a. PTHREAD_MUTEX_ADAPTIVE_NP, which enables a spin loop pthread_mutex_lock() in the GNU libc.

We used to have a spinlock for this log_lsn_lock. The performance was truly horrible, as tested on our normal, non-NUMA machines.

One feasible solution would be to have multiple implementations and choose the preferred one by assigning a function pointer on startup. First we need to find a minimal tweak that improves things for NUMA.

The performance impact of a few ideas is still being tested.

steve.shaw@intel.com has experimented with the addition of a redundant pure spin lock, first around mtr_t::do_write() and on my suggestion, around mtr_t::finish_write(). The redundant spin lock would prevent conflicts on log_sys.lsn_lock and therefore the costly context switches between user and kernel space. This will improve performance also on a single-socket setup (no NUMA).

Based on this, I will try to replace log_sys.lsn_lock with a pure spin lock. That is something that we never tried so far. The srw_spin_lock would still fall back to something like futex_wait() after an initial spin loop.

We had tried lsn_lock as pure spin lock, but this gets consistently forgotten

MDEV-27866 has this comment, on Feb 20 2022
-----------------------------------------------
The critical section of lsn_lock is tiny, yet when pure spinlock was used, it slowed thing down on everything. NUMA, non-NUMA, single socket, Windows. I do not have NUMA, but on 16CPU Windows, the 256 users update_index went down from 220-230ktps to 160ktps, while IIRC on Marko Mäkelä's dual socket, it went down from 200 ktps to 30ktps in the same test.

It is not entirely obvious why this happened, I'd check memory access pattern
------------------------------------------------

wlad, thank you for reminding me. In MDEV-27866 I found a pointer to the mcspin_lock patch that I had tested back then (over 2 years ago). It is based on https://gitlab.com/numa-spinlock/numa-spinlock and includes some queue component. The spin lock that steve.shaw@intel.com tested is much simpler, just one lock word. The results with that could be quite different. There has been at least one memory layout improvements since that previous experiment: MDEV-32374.

I created two patches. On my Haswell microarchitecture dual Intel Xeon E5-2630 v4, both result in significantly worse throughput with 256-thread Sysbench oltp_update_index (actually I intended to test oltp_update_non_index) than the 10.11 baseline. With the baseline, the Linux kernel function native_queued_spin_lock_slowpath is the busiest one; with either fix, it would end up in the second place, behind the new function lsn_delay().

The first variant more closely resembles what steve.shaw@intel.com did. It uses std::atomic<bool> for the lock word; the lock acquisition would be xchg.

The second variant merges log_sys.lsn_lock to the most significant bit of log_sys.buf_free. Lock acquisition will be a loop around lock cmpxchg. It yields better throughput on my system than the first variant. One further thing that could be tried would be a combination of lock bts and a separate mov to load the log_sys.buf_free value. On ARMv8, POWER, RISC-V or other modern ISA, we could probably use simple std::atomic::fetch_or().

I think that some further testing on newer Intel microarchitectures is needed to determine whether I am on the right track with these.

I tried the patch on AlderLake (server running on P-cores, sysbench on E-cores)
update_index_256threads_x_10tables_x_1mio_rows.svg

In short, spinlocks do make performance worse, e.g TPS for 1 minute update_index x256 threads x10 tables x 1mio rows (in memory) looks like this

I also added spinflag.svg and baseline.svg flamegraphs, so one can see what's going on.
Apparently, the append_prepare/lsn_delay takes half of the time 48%, in "spin" variation, on Alder Lake, while in baseline, append_prepare is barely noticable, with 0.5%

It is not just Haswell, that performs badly, and this sorta matches what we had seen 2 years ago.

https://github.com/MariaDB/server/pull/3148 introduces SET GLOBAL innodb_log_spin_wait_delay, which can be used to enable or disable the spin lock while the server is running. The value 50 should roughly correspond to what the previous spinflag patch did. The default value innodb_log_spin_wait_delay=0 means that the log_sys.lsn_lock will be used. I think that we must rely on steve.shaw@intel.com to test this on Emerald Rapids.

I have attached on a test Emerald Rapids system with 56 core CPUs with both 1 and 2 socket tests, this improves performance by 9% (1 socket) and 12% (2 socket) respectively and the highest MariaDB throughput we have measured from any release so far with very stable performance measured throughout each of this tests.

I think that it would be interesting to know the limits on the number of concurrent threads on more recent microarchitectures. Common sense would suggest that spinning would work better when the number of concurrent threads is limited to a fraction of the number of hardware threads. I did not test such low concurrency on my Haswell microarchitecture system.

In any case, it is easy to tune this if it does not work, simply by SET GLOBAL innodb_log_spin_wait_delay.