[libc][NFC] Simplifly inbox and outbox state handling

[jhuber6]

Summary

Currently we use a template parameter called InvertInbox to invert the
inbox when we load it. This is more easily understood as a static check
on whether or not the process running it is the server. Inverting the
inbox makes the states 1 0 and 0 1 own the buffer, so it's easier to
simply say that the server own the buffer if in != out. Also clean up some of
the comments.

[JonChesterfield]

Different definitions of simple in play here.

If

• one reasons in terms of the values in the underlying memory
• focuses on the differences between client and server
• prefers code duplication

Then this is a reasonable way to go.

The direction I'm trying to take this in is different.

• move atomic ops and fences out of client/server
• concurrency logic in process is the same for either instance
• client/server only work with concepts libc cares about

That has the advantage that the code in process covers all the subtle concurrency hazards and can be tested in isolation, notably stress tested under thread sanitisers with both sides running on x64. It also means the libc send/recv stuff can evolve over time more easily as it doesn't have to worry about breaking the concurrency model.

It also has the advantage that inbox and outbox always mean exactly the same thing, on client or server or process, which I find far easier to reason about than keeping track of which is the root of a given function call.

I'm ok with not thinking about whether a given block of memory underlying a mailbox has particular numbers written into it, in exchange for thinking about the states the machine is in.

[jhuber6]

Update to make this more obvious that we are simply moving the inversion from
the load to the condition. The inversion affects the condition so it is more
intuitive for the condition itself to be inverted. That is !a == b is the same
as !(a == b).

[JonChesterfield]

I'm not convinced. This means reasoning in terms of the values in memory instead of in the abstract state of a process. However the change is localised and easy to reverse if it interacts badly with future patches and you seem to be especially passionate about this.

libc/src/__support/RPC/rpc.h

 
 // TODO: This should be configured by the server and passed in. The general rule
 //       of thumb is that you should have at least as many ports as possible
 //       concurrent work items on the GPU to mitigate the lack offorward
 //       progress guarantees on the GPU.
 constexpr uint64_t DEFAULT_PORT_COUNT = 64;
 
 /// A common process used to synchronize communication between a client and a
-/// server. The process contains an inbox and an outbox used for signaling
-/// ownership of the shared buffer between both sides.
+/// server. The process contains a read-only inbox and a write-only outbox used
+/// for signaling ownership of the shared buffer between both sides. We assign
+/// ownership of the buffer to the client if the inbox and outbox bits match,
+/// otherwise it is owned by the server.
 ///
-/// No process writes to its inbox. Each toggles the bit in the outbox to pass
-/// ownership to the other process.
-/// When inbox == outbox, the current state machine owns the buffer.
-/// Initially the client is able to open any port as it will load 0 from both.
-/// The server inbox read is inverted, so it loads inbox==1, outbox==0 until
-/// the client has written to its outbox.
-///
-/// This process is designed to support mostly arbitrary combinations of 'send'
-/// and 'recv' operations on the shared buffer as long as these operations are
-/// mirrored by the other process. These operations exchange ownership of the
-/// fixed-size buffer between the users of the protocol. The assumptions when
-/// using this process are as follows:
-///   - The client will always start with a 'send' operation
-///   - The server will always start with a 'recv' operation
-///   - For every 'send' / 'recv' call on one side of the process there is a
-///     mirrored 'recv' / 'send' call.
-///
-template <bool InvertInbox> struct Process {
+/// This process is designed to allow the client and the server to exchange data
+/// using a fixed size packet in a mostly arbitrary order using the 'send' and
+/// 'recv' operations. The following restrictions to this scheme apply:
+///   - The client will always start with a 'send' operation.
+///   - The server will always start with a 'recv' operation.
+///   - Every 'send' or 'recv' call is mirrored by the other process.
+template <bool Invert> struct Process {
   LIBC_INLINE Process() = default;
   LIBC_INLINE Process(const Process &) = delete;
   LIBC_INLINE Process &operator=(const Process &) = delete;
   LIBC_INLINE Process(Process &&) = default;
   LIBC_INLINE Process &operator=(Process &&) = default;
   LIBC_INLINE ~Process() = default;
 
   uint64_t port_count;
template <bool Invert> struct Process {
   /// The length of the packet is flexible because the server needs to look up
   /// the lane size at runtime. This helper indexes at the proper offset.
   LIBC_INLINE Packet &get_packet(uint64_t index) {
     return *reinterpret_cast<Packet *>(advance(
         packet, index * align_up(sizeof(Header) + lane_size * sizeof(Buffer),
                                  alignof(Packet))));
   }
 
-  /// Inverting the bits loaded from the inbox in exactly one of the pair of
-  /// processes means that each can use the same state transitions.
-  /// Whichever process has InvertInbox==false is the initial owner.
-  /// Inbox equal Outbox => current process owns the buffer
-  /// Inbox difer Outbox => current process does not own the buffer
-  /// At startup, memory is zero initialised and raw loads of either mailbox
-  /// would return zero. Thus both would succeed in opening a port and data
-  /// races result. If either inbox or outbox is inverted for one process, that
-  /// process interprets memory as Inbox!=Outbox and thus waits for the other.
-  /// It is simpler to invert reads from the inbox than writes to the outbox.
+  /// Retrieve the inbox state from memory shared between processes.
   LIBC_INLINE uint32_t load_inbox(uint64_t index) {
-    uint32_t i = inbox[index].load(cpp::MemoryOrder::RELAXED);
-    return InvertInbox ? !i : i;
+    return inbox[index].load(cpp::MemoryOrder::RELAXED);
   }
 
   /// Retrieve the outbox state from memory shared between processes.
-  /// Never needs to invert the associated read.
   LIBC_INLINE uint32_t load_outbox(uint64_t index) {
     return outbox[index].load(cpp::MemoryOrder::RELAXED);
   }
 
   /// Signal to the other process that this one is finished with the buffer.
   /// Equivalent to loading outbox followed by store of the inverted value
   /// The outbox is write only by this warp and tracking the value locally is
   /// cheaper than calling load_outbox to get the value to store.
   LIBC_INLINE uint32_t invert_outbox(uint64_t index, uint32_t current_outbox) {
     uint32_t inverted_outbox = !current_outbox;
     outbox[index].store(inverted_outbox, cpp::MemoryOrder::RELAXED);
     return inverted_outbox;
   }
 
-  /// Determines if this process needs to wait for ownership of the buffer.
+  /// Determines if this process needs to wait for ownership of the buffer. We
+  /// invert the condition on one of the processes to indicate that if one
+  /// process owns the buffer then the other does not.
   LIBC_INLINE static bool buffer_unavailable(uint32_t in, uint32_t out) {
-    return in != out;
+    bool cond = in != out;
+    return Invert ? !cond : cond;
   }
 
   /// Attempt to claim the lock at index. Return true on lock taken.
   /// lane_mask is a bitmap of the threads in the warp that would hold the
   /// single lock on success, e.g. the result of gpu::get_lane_mask()
   /// The lock is held when the zeroth bit of the uint32_t at lock[index]
   /// is set, and available when that bit is clear. Bits [1, 32) are zero.
   /// Or with one is a no-op when the lock is already held.
return is_process_gpu()
                ? port_count * sizeof(Packet)
                : port_count *
                      align_up(sizeof(Header) + (lane_size * sizeof(Buffer)),
                               alignof(Packet));
   }
 
   /// Offset of the inbox in memory. This is the same as the outbox if inverted.
   LIBC_INLINE static uint64_t inbox_offset(uint64_t port_count) {
-    return InvertInbox ? mailbox_bytes(port_count) : 0;
+    return Invert ? mailbox_bytes(port_count) : 0;
   }
 
   /// Offset of the outbox in memory. This is the same as the inbox if inverted.
   LIBC_INLINE static uint64_t outbox_offset(uint64_t port_count) {
-    return InvertInbox ? 0 : mailbox_bytes(port_count);
+    return Invert ? 0 : mailbox_bytes(port_count);
   }
 
   /// Offset of the buffer containing the packets after the inbox and outbox.
   LIBC_INLINE static uint64_t buffer_offset(uint64_t port_count) {
     return align_up(2 * mailbox_bytes(port_count), alignof(Packet));
   }
 };