Sergey Vojtovich added a comment - 2016-05-03 08:45 serg , I merged everything I found reasonable. Could you have another look at this PR? Merged changes are here: https://github.com/MariaDB/server/commits/bb-10.2-mdev9857

Sergey Vojtovich added a comment - 2016-06-02 08:34 License confirmed. There's also PR#178 now.

Sergei Golubchik added a comment - 2016-06-06 14:51 Looks ok. The only comment is about /* we want sizeof(LF_PINS) to be CPU_LEVEL1_DCACHE_LINESIZE * 2 to avoid false sharing */ char pad[CPU_LEVEL1_DCACHE_LINESIZE * 2 - sizeof (uint32) * 2 - sizeof (LF_PINBOX*) - sizeof ( void *) - sizeof ( void *) * (LF_PINBOX_PINS + 1 This is not exactly true. We don't want sizeof(LF_PINS) to be CPU_LEVEL1_DCACHE_LINESIZE * 2 , we want sizeof(LF_PINS) % CPU_LEVEL1_DCACHE_LINESIZE == 0 , but whether it'll be two cache lines, one cache line, three cache lines or ten cache lines — doesn't matter much. Preferably, as few as possible. With cache line size being 64, I had to use 64*2, because LF_PINS didn't fit in 64 bytes. But with cache line size being 128, LF_PINS will fit in one cache line, there's no need to allocate 256 bytes for it. It'd be perfect if you could change it to use as few cache lines as possible.

Daniel Black added a comment - 2016-06-07 01:52 PR#178 removed padding as a mechanism for alignment in favour of using compiler alignment combined with malloc alignment to keep these on different cache lines. Test case to show the alignment of the structure member affects the structure size: https://gist.github.com/grooverdan/19a7c8d8ac0e680ba6fcf4668962b9c4 The original pad calculation that did include a mod - https://github.com/MariaDB/server/pull/169/files#diff-41202b17494e25e891bbfa27df165331R71 As the pad isn't actually referenced in the code, additional size just means wasted memory.

Sergey Vojtovich added a comment - 2016-06-07 09:03 - edited serg , remember our discussion re paddings: svoj> > > > > + struct st_locks svoj> > > > > + { svoj> > > > > + mysql_rwlock_t lock; svoj> > > > > + char pad[128]; serg> > > > serg> > > > In these cases I usually do: pad[128 - sizeof(mysql_rwlock_t)]; svoj> > > Hmm... strange that I didn't define macro for this, I thought I did. svoj> > > svoj> > > Your suggestion guarantees that st_locks will be at least 128 bytes. But these svoj> > > 128 bytes may be split across 2 cache lines. Or malloc returns pointer aligned svoj> > > on cache line size? serg> > serg> > No, it doesn't. As far as I know. But I only care that different locks serg> > go into different cache lines, they don't need to be aligned to 128 byte serg> > boundary. svoj> What about this: svoj> sizeof(mysql_rwlock_t)= 64 (just rough estimation) svoj> sizeof(pad[128 - sizeof(mysql_rwlock_t)])= 64 svoj> svoj> It gives: svoj> +-------------------------------+-------------+-------------------------------+ svoj> | rw_lock_t 64b | padding 64b | rw_lock_t 64b | svoj> +---------------+---------------+-------------+---------------+---------------+ svoj> Cache line 128b | Cache line 128b |Cache line 128b svoj> ----------------+---------------------------------------------+---------------- serg> Okay, indeed. You're right. Your statetement is correct for aligned allocs. Minimal padding in this particular case is "char pad[CPU_LEVEL1_DCACHE_LINESIZE]". I'll fix it accordingly.

Daniel Black added a comment - 2016-06-29 23:33 If you have aligned allocs and and aligned structure elements do you actually still need padding? ( https://github.com/MariaDB/server/pull/185 ).

Sergey Vojtovich added a comment - 2016-06-30 06:42 No padding for aligned structures is needed.