Simplify core 2 #13

yilongli · 2020-10-28T05:33:38Z

Made some more improvements atop #11.

…ation Details: - Disable DpdkDriver in CMakeLists.txt temporarily - Remove unused method Driver::Packet::getMaxPayloadSize() - Remove param `driver` in handleXXXPacket - Change Driver::Packet to become a POD struct - Change Driver::Packet::{address,priority} into params in Driver::sendPacket - Remove the opaque Driver::Address - Use IP packets as the common interface between transport and driver - Extend Homa packet headers to include L4 src/dst port numbers - Use SocketAddress (i.e., ip + port) as opposed to Driver::Address to identify the src/dst address of a message

- Reenable DPDK in CMakeLists.txt - Initialize an ARP table at driver startup using the content of /proc/net/arp - Select eth port via the symbolic name of the network interface (e.g., eno1d1) (the current implementation uses ioctl to obtain the IP and MAC addresses of a network interface) - Add a system test for DPDK driver: test/dpdk_test.cc

- Change IpAddress from typedef to a POD type to provide some type safety - Add a third argument to Driver::receivePackets() to hold source addresses of ingress packets when the method returns - Eliminate Driver::Packet (use Homa::PacketSpec instead) - Move L4 header fields sport/dport into header prefix

Timeouts are now checked incrementally with one bucket checked every poll iteration. Previously, timeout checking was more concentrated; checking was triggered less frequently but all buckets would be checked once triggered. The previous design allows the poll iteration to complete quickly when no timeout checking is triggered but significantly increases poll() execution times when checking needs to occur. This causes large, periodic latency spikes when using the transport. In the new design sightly increases the minimum poll() execution times but allows the work of checking timeouts to be more evenly distributed over time and thus reduces latency spikes.

Homa will now ensure that an OutMessage is fully sent before it is cleaned up if an application releases the message after calling send(). Previously, released messages that were in progress of being sent were cancelled and immediately cleaned up. Canceling a message now requires an explicit call to cancel().

Make the Perf::Counters::Stat::add(T val) operation thread unsafe. This significantly reduces the overhead of updating a Stat and is ok because add() is only called on thread-local Stat instances.

Previously, the number of messages completed per call to trySend() was limited by an std::array allocated on the stack. This was done to reduce the allocation overhead. New Perf microbenchmarks show that reserving capacity in an std::vector is likely efficient enough and would allow trySend() to seamlessly handle an unlimited number of message completions per call to trySend().

The Sender will now skip pinging a message if the message still has granted but unsent packets (e.g. it is waiting on itself).

Will be used in future integration with non-polling interface.

Will be used by a future non-polling interface.

…sed on Collin's comments

cstlee

The clean up, especially of the interfaces, looks pretty good; I don't have any issues there. My main concern is with the locking.

Overall, it seems like this PR reduces the size/scope of the bucket and queue/scheduler mutex critical sections in most places. Unfortunately, doing this seems to have introduced potential race conditions and TOCTOU bugs. Perhaps I missed some critical factors that rule out these issues, but the locking did not make it obvious that the code is race-free. I would argue that using a locking style more like monitors with the larger critical sections makes it easier to convince ourselves that the logic is thread-safe. What was the motivation for using fine-grain critical sections? Can we achieve your goals and still have obviously thread-safe code? Let's find a time to discuss.

cstlee · 2020-11-03T18:52:53Z

src/Drivers/DPDK/DpdkDriverImpl.cc

@@ -179,7 +177,6 @@ Driver::Packet*
 DpdkDriver::Impl::allocPacket()
 {
    DpdkDriver::Impl::Packet* packet = nullptr;
-    SpinLock::Lock lock(packetLock);
    static const int MBUF_ALLOC_LIMIT = NB_MBUF - NB_MBUF_RESERVED;
    if (mbufsOutstanding < MBUF_ALLOC_LIMIT) {


mbufsOutstanding is now unprotected. Have you double checked to make sure there aren't syncronization and TOCTOU issues? At the very least, it should probablly be declared atomic.

I decided to make the thread-safety of ObjectPool configurable via a template argument, which is generally useful anyway. Since I am lazy, I am going to revert all changes on DpdkDriver.

src/Drivers/DPDK/DpdkDriverImpl.cc

src/ObjectPool.h

cstlee · 2020-11-03T19:04:10Z

src/Timeout.h

    {
        list.remove(&timeout->node);
-        timeout->expirationCycleTime =
-            PerfUtils::Cycles::rdtsc() + timeoutIntervalCycles;
+        timeout->expirationCycleTime = now + timeoutIntervalCycles;


Using a caller provided now is problematic. There is no gaurantee that the caller will provide monotically increasing now values, which would break the timer list's ordering invariant.

Got it; revert the change. By the way, this method won't be called very often anyway once my changes on the timeout mechanism are merged.

src/Sender.h

cstlee · 2020-11-10T00:08:28Z

src/Receiver.cc

+    {
+        SpinLock::Lock lock_bucket(bucket->mutex);
+        message = bucket->findMessage(id, lock_bucket);
+    }


Message might be invalid after bucket mutex release.

Extended the critical section to the entire method.

src/Receiver.h

cstlee · 2020-11-10T00:17:18Z

src/Receiver.cc

-    // Things that must be true (sanity check)
-    assert(id == message->id);
-    assert(message->driver == driver);
+    // Copy the payload into the message buffer.


Why copy the data out while processing the packet instead of when the application calls receive()? I can think of potential pros and cons but I'm wondering what you are considering.

If you want to be able to release the packet buffer back to the driver when this method returns, you would have to do the copy here. In general, it's going to be faster to do copy while the packet data is still hot; this also helps reduce the number of RX buffers in use in the driver.

cstlee · 2020-11-10T00:22:42Z

src/Receiver.cc

+        message->numResendTimeouts++;
+        if (message->numResendTimeouts >= MESSAGE_TIMEOUT_INTERVALS) {
+            // Message timed out before being fully received; drop the message.
+            messageAllocator.destroy(message);


Does this deadlock on ~Message()? The bucket->mutex is held and also used the destructor, correct?

Yeah, good catch. Now I used a UniqueLock and released the lock before calling dropMessage (the alternative is to duplicate functionality from dropMessage like the original code). Yes, I also revived dropMessage because it feels like information leakage to detach a message from the transport inside its destructor.

src/Receiver.cc

…bucket mutex

yilongli · 2020-11-12T06:40:12Z

The clean up, especially of the interfaces, looks pretty good; I don't have any issues there. My main concern is with the locking.

Overall, it seems like this PR reduces the size/scope of the bucket and queue/scheduler mutex critical sections in most places. Unfortunately, doing this seems to have introduced potential race conditions and TOCTOU bugs. Perhaps I missed some critical factors that rule out these issues, but the locking did not make it obvious that the code is race-free. I would argue that using a locking style more like monitors with the larger critical sections makes it easier to convince ourselves that the logic is thread-safe. What was the motivation for using fine-grain critical sections? Can we achieve your goals and still have obviously thread-safe code? Let's find a time to discuss.

There are three advantages with the fine-grain critical section approach. First, the code is more explicit about what need to be protected. Second, we can flip the locking order constraint to be queueMutex before bucket mutex and make the code in trySend() simpler. Third, it becomes easier to change the code to make sure no locks are held when calling into the driver. Obviously, I missed the point that the original code was also using the bucket mutex to achieve safe memory reclamation. I decided not to try the reference counting approach so I have now reverted to the original design (at least for Sender, the problems in Receiver seems rather easy to fix).

cstlee

Looks good. Mostly small comments. The possible remaining discussion topics are:

John's comments about the copying interface.
Test coverage
C++ version
Next steps

cstlee · 2020-11-17T22:59:11Z

src/ObjectPool.h

@@ -99,38 +102,66 @@ class ObjectPool {
    template <typename... Args>
    T* construct(Args&&... args)
    {
-        void* backing = NULL;
+        void* backing = nullptr;
+        enterCS();


I like this idea of making the locking optional. These methods are small so I doesn't matter all that much but I'm wondering if there is an easy RAII version of locking and unlocking; they tend to be less error prone.

cstlee · 2020-11-17T23:03:46Z

src/Receiver.cc

+        // A new Message needs to be entered into the scheduler.
+        SpinLock::Lock lock_scheduler(schedulerMutex);
+        schedule(message, lock_scheduler);
+    } else if (message->scheduled) {


Line 148 already says if (message->scheduled). One of these lines is probably wrong. Does this patch include test coverage? If so, a unit test is likely missing to test this case.

cstlee · 2020-11-17T23:06:53Z

src/Receiver.cc

+
+        // Deliver the message to the user of the transport.
+        SpinLock::Lock lock_received_messages(receivedMessages.mutex);
+        receivedMessages.queue.push_back(&message->receivedMessageNode);


Can we safely access the message's members without the bucket_lock? If so, it should be documented as to why.

cstlee · 2020-11-18T01:24:38Z

src/Receiver.cc

@@ -785,30 +599,27 @@ Receiver::trySendGrants()
 *      Reminder to hold the Receiver::schedulerMutex during this call.
 */
 void
-Receiver::schedule(Receiver::Message* message, const SpinLock::Lock& lock)
+Receiver::schedule(Receiver::Message* message,
+                   [[maybe_unused]] SpinLock::Lock& lock)


What version of C++/gcc are you assuming? The project currently checks for C++11.

yilongli and others added 30 commits October 6, 2020 01:56

Fixed compilation erros in src/Perf.h

234e2ab

fixed copyrights and formatting issues

c1308dc

Include additional Perf microbenchmarks

a75e59c

Clean up comments

679e483

Add Perf benchmark atomicIncUnsafe

dd631fe

Remove unneeded Perf::Counters:Stat thread-safety

3ce5233

Make the Perf::Counters::Stat::add(T val) operation thread unsafe. This significantly reduces the overhead of updating a Stat and is ok because add() is only called on thread-local Stat instances.

Clean up Perf::Timer usage

ffab71e

Add more Perf microbenchmarks

020468e

Simplify DpdkDriver Packet allocation

793de5a

Change Sender to not PING when blocked on itself

e5d45bd

The Sender will now skip pinging a message if the message still has granted but unsent packets (e.g. it is waiting on itself).

Drop message from sendQueue on Message timeout

c78f1d9

Add Message statistics

610b27a

DpdkDriver bug fixes and improvements

3c6b2e1

Prevent DpdkDriver loopback starvation

429db01

Bump version to 0.2.0.0

55c3e34

Add missing DpdkDriverImpl member init

e143c27

Expose Receiver::checkTimeouts() as public method

b617c47

Will be used in future integration with non-polling interface.

Expose Sender::checkTimeouts() as public method.

40345af

Will be used by a future non-polling interface.

Minor cleanups in Sender.h

062fa25

wip: complete cleanups for Sender

e788958

completed the first draft of new Sender

8859e05

final cleanup on Sender

ee3db75

similar cleanups for Receiver

c382ea3

pulled in the changes that simplify Homa::InMessage + improvements ba…

1973fdd

…sed on Collin's comments

yilongli requested a review from cstlee October 28, 2020 05:33

cstlee requested changes Nov 10, 2020

View reviewed changes

yilongli added 3 commits November 10, 2020 00:42

fixed some trivial issues raised in the code reviews

6969f25

fixed the race conditions in Receiver due to reduced coverage of the …

ca0a7cb

…bucket mutex

fixed race conditions on the Sender side

f3799b9

cstlee reviewed Nov 18, 2020

View reviewed changes

cstlee force-pushed the master branch from 40345af to 392b577 Compare January 24, 2021 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify core 2 #13

Simplify core 2 #13

yilongli commented Oct 28, 2020

cstlee left a comment

cstlee Nov 3, 2020

yilongli Nov 10, 2020

cstlee Nov 3, 2020

yilongli Nov 10, 2020

cstlee Nov 10, 2020

yilongli Nov 11, 2020

cstlee Nov 10, 2020

yilongli Nov 10, 2020

cstlee Nov 10, 2020

yilongli Nov 10, 2020

yilongli commented Nov 12, 2020

cstlee left a comment

cstlee Nov 17, 2020

cstlee Nov 17, 2020

cstlee Nov 17, 2020

cstlee Nov 18, 2020

Simplify core 2 #13

Are you sure you want to change the base?

Simplify core 2 #13

Conversation

yilongli commented Oct 28, 2020

cstlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yilongli commented Nov 12, 2020

cstlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment