> 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`.
>
> Dario Rexin wrote:
> 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.
I concur with Dario that it's better to be explicit about the padding. This
would ensure no false sharing occurs when a `MpscLinkedQueue` instance is
followed by another variable that has no explicit alignment. We should capture
this rationale (and a summary of this discussion) in a code comment.
FWIW, the only usage of `MpscLinkedQueue` is:
```C
class EventQueue
{
MpscLinkedQueue<Event> queue;
std::atomic<bool> comissioned = ATOMIC_VAR_INIT(true);
};
```
Experimentally, gcc doesn't add the padding we want in this case:
```C
#include <atomic>
#include <stdio.h>
#include <stddef.h>
struct Event
{
alignas(64) std::atomic<void *> p1;
alignas(64) std::atomic<void *> p2;
std::atomic<bool> b;
};
Event e;
int main()
{
printf("p1 -> %zu\n", offsetof(Event, p1));
printf("p1 -> %zu\n", offsetof(Event, p2));
printf("b -> %zu\n", offsetof(Event, b));
}
$ c++ -std=c++14 ./e.cc && ./a.out
p1 -> 0
p1 -> 64
b -> 72
```
- James
-----------------------------------------------------------
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
>
>
