> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote: > > 3rdparty/libprocess/src/mpsc_linked_queue.hpp > > Lines 184-188 (patched) > > <https://reviews.apache.org/r/68001/diff/1/?file=2061817#file2061817line186> > > > > Wouldn't it be possible to avoid all the manual padding by aligning > > both `head` and `tail` on their own cache lines? > > > > // We align `head` to 64 bytes (x86 cache line size) to guarantee > > it to > > // be put on a new cache line. This is to prevent false sharing with > > // other objects that could otherwise end up on the same cache line > > as > > // this queue. We also align `tail` to avoid false sharing of `head` > > // with `tail` and to avoid false sharing with other objects. > > > > // We assume a x86_64 cache line size of 64 bytes. > > // > > // TODO(drexin): Programatically get the cache line size. > > #define CACHE_LINE 64 > > > > alignas(CACHE_LINE) std::atomic<Node<T>*> head; > > alignas(CACHE_LINE) Node<T>* tail; > > > > #undef CACHE_LINE > > > > Wouldn't this satisfy the guarantees you list in your comment? > > > > It would also allows us to avoid the macro which has a number of issues > > (`__COUNTER__` is a GNU extension, missing check of `sizeof(var) < 64`). > > Benjamin Bannier wrote: > I should have used a real variable for `CACHE_LINE` above to make this > less nasty, e.g., > > static constexpr std::size_t CACHE_LINE = 64; // Requires e.g., > `#include <cstddef>`. > > Dario Rexin wrote: > It would probably work for the padding between head and tail, but what > about the padding after tail? Should we `alignas(64) char tailPad;` or > somethign like that? > > Benjamin Bannier wrote: > If we align both `head` and `tail` on separate cache lines, I believe we > cannot have one queue's `tail` share a cache line with e.g., another queue's > `head` > > Do need to worry about `tail` sharing a cache line with arbitrary other > data? If not it would seem we wouldn't need additional padding after `tail`.
In general we should worry about other data. Anything that can cause cache line invalidation would be problematic. I don't want to make assumptions about the likelyhood of that happening. I think it's better to be on the safe side. - Dario ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68001/#review206289 ----------------------------------------------------------- On July 20, 2018, 4:51 p.m., Dario Rexin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68001/ > ----------------------------------------------------------- > > (Updated July 20, 2018, 4:51 p.m.) > > > Review request for Benjamin Bannier. > > > Repository: mesos > > > Description > ------- > > This patch aligns the head of MpscLinkedQueue to a new cache line > and adds padding between head and tail to avoid false sharing > between to two and after tail to avoid false sharing with other > objects that could otherwise end up on the same cache line. > > > Diffs > ----- > > 3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d > > > Diff: https://reviews.apache.org/r/68001/diff/1/ > > > Testing > ------- > > make check & benchmarks > > > Thanks, > > Dario Rexin > >
