mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dario Rexin <da...@mesosphere.io>
Subject Re: Review Request 68001: Fix padding in MpscLinkedQueue.
Date Mon, 23 Jul 2018 20:50:06 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message