Improvements based on code review discussions with Collin

- 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

Author: yilongli

CMakeLists.txt

@@ -1,6 +1,6 @@
 cmake_minimum_required (VERSION 3.11)
 
-project(Homa VERSION 0.1.1.0 LANGUAGES CXX)
+project(Homa VERSION 0.1.2.0 LANGUAGES CXX)
 
 ################################################################################
 ## Dependency Configuration ####################################################
@@ -74,6 +74,7 @@ endif()
 add_library(Homa
     src/CodeLocation.cc
     src/Debug.cc
+    src/Driver.cc
     src/Homa.cc
     src/Perf.cc
     src/Policy.cc

include/Homa/Driver.h

@@ -22,8 +22,49 @@
 
 namespace Homa {
 
-/// IPv4 address in host byte order.
-using IpAddress = uint32_t;
+/**
+ * A simple wrapper struct around an IP address in binary format.
+ *
+ * This struct is meant to provide some type-safety when manipulating IP
+ * addresses. In order to avoid any runtime overhead, this struct contains
+ * nothing more than the IP address, so it is trivially copyable.
+ */
+struct IpAddress final {
+    /// IPv4 address in host byte order.
+    uint32_t addr;
+
+    /**
+     * Unbox the IP address in binary format.
+     */
+    explicit operator uint32_t()
+    {
+        return addr;
+    }
+
+    /**
+     * Equality function for IpAddress, for use in std::unordered_maps etc.
+     */
+    bool operator==(const IpAddress& other) const
+    {
+        return addr == other.addr;
+    }
+
+    /**
+     * This class computes a hash of an IpAddress, so that IpAddress can be used
+     * as keys in unordered_maps.
+     */
+    struct Hasher {
+        /// Return a "hash" of the given IpAddress.
+        std::size_t operator()(const IpAddress& address) const
+        {
+            return std::hash<typeof(addr)>{}(address.addr);
+        }
+    };
+
+    static std::string toString(IpAddress address);
+    static IpAddress fromString(const char* addressStr);
+};
+static_assert(std::is_trivially_copyable<IpAddress>());
 
 /**
  * Represents a packet of data that can be send or is received over the network.
@@ -43,6 +84,7 @@ struct PacketSpec {
     /// Number of bytes in the payload.
     int32_t length;
 }  __attribute__((packed));
+static_assert(std::is_trivial<PacketSpec>());
 
 /**
  * Used by Homa::Transport to send and receive unreliable datagrams.  Provides
@@ -52,41 +94,8 @@ struct PacketSpec {
  */
 class Driver {
   public:
-    /**
-     * Represents a packet that can be send or is received over the network.
-     *
-     * The layout of this struct has two parts: the first part is essentially
-     * a copy of PacketSpec, while the second part contains members specific
-     * to our driver implementation.
-     *
-     * @sa Homa::PacketSpec
-     */
-    struct Packet final {
-        // === PacketSpec definitions ===
-        // The order and types of the following members must match those in
-        // PacketSpec precisely.
-
-        /// See Homa::PacketSpec::payload.
-        void* payload;
-
-        /// See Homa::PacketSpec::length
-        int32_t length;
-
-        // === Extended definitions ===
-        // The following members are specific to the driver framework bundled
-        // in this library. Therefore, these members must *NOT* appear in the
-        // core components of Homa transport; they are only used in a few
-        // places to facilitate the glue code between transport and driver.
-
-        /// Packet's source IpAddress. Only meaningful when this packet is an
-        /// incoming packet.
-        IpAddress sourceIp;
-    }  __attribute__((packed));
-
-    // Static checks to enforce the object layout compatibility between
-    // Driver::Packet and PacketSpec.
-    static_assert(offsetof(Packet, payload) == offsetof(PacketSpec, payload));
-    static_assert(offsetof(Packet, length) == offsetof(PacketSpec, length));
+    /// Import PacketSpec into the Driver namespace.
+    using Packet = PacketSpec;
 
     /**
      * Driver destructor.
@@ -164,14 +173,18 @@ class Driver {
      * this method.
      * @param[out] receivedPackets
      *      Received packets are appended to this array in order of arrival.
+     * @param[out] sourceAddresses
+     *      Source IP addresses of the received packets are appended to this
+     * array in order of arrival.
      *
      * @return
      *      Number of Packet objects being returned.
      *
      * @sa Driver::releasePackets()
      */
     virtual uint32_t receivePackets(uint32_t maxPackets,
-                                    Packet* receivedPackets[]) = 0;
+                                    Packet* receivedPackets[],
+                                    IpAddress sourceAddresses[]) = 0;
 
     /**
      * Release a collection of Packet objects back to the Driver. Every

include/Homa/Drivers/DPDK/DpdkDriver.h

@@ -133,7 +133,8 @@ class DpdkDriver : public Driver {
 
     /// See Driver::receivePackets()
     virtual uint32_t receivePackets(uint32_t maxPackets,
-                                    Packet* receivedPackets[]);
+                                    Packet* receivedPackets[],
+                                    IpAddress sourceAddresses[]);
 
     /// See Driver::releasePackets()
     virtual void releasePackets(Packet* packets[], uint16_t numPackets);

include/Homa/Drivers/Fake/FakeDriver.h

@@ -58,24 +58,27 @@ struct FakePacket {
     /// Raw storage for this packets payload.
     char buf[MAX_PAYLOAD_SIZE];
 
+    /// Source IpAddress of the packet.
+    IpAddress sourceIp;
+
     /**
      * FakePacket constructor.
      */
     explicit FakePacket()
         : base{.payload = buf,
-               .length = 0,
-               .sourceIp = 0}
+               .length = 0}
         , buf()
+        , sourceIp()
     {}
 
     /**
      * Copy constructor.
      */
     FakePacket(const FakePacket& other)
         : base{.payload = buf,
-               .length = other.base.length,
-               .sourceIp = 0}
+               .length = other.base.length}
         , buf()
+        , sourceIp()
     {
         memcpy(base.payload, other.base.payload, MAX_PAYLOAD_SIZE);
     }
@@ -111,7 +114,8 @@ class FakeDriver : public Driver {
     virtual Packet* allocPacket();
     virtual void sendPacket(Packet* packet, IpAddress destination, int priority);
     virtual uint32_t receivePackets(uint32_t maxPackets,
-                                    Packet* receivedPackets[]);
+                                    Packet* receivedPackets[],
+                                    IpAddress sourceAddresses[]);
     virtual void releasePackets(Packet* packets[], uint16_t numPackets);
     virtual int getHighestPacketPriority();
     virtual uint32_t getMaxPayloadSize();
@@ -121,7 +125,7 @@ class FakeDriver : public Driver {
 
   private:
     /// Identifier for this driver on the fake network.
-    uint64_t localAddressId;
+    uint32_t localAddressId;
 
     /// Holds the incoming packets for this driver.
     FakeNIC nic;

include/Homa/Util.h

@@ -22,10 +22,11 @@
 #include <string>
 
 /// Cast a member of a structure out to the containing structure.
-#define container_of(ptr, type, member) ({ \
-    const typeof( ((type *)0)->member ) \
-    *__mptr = (ptr); \
-    (type *)( (char *)__mptr - offsetof(type,member) );})
+template<class P, class M>
+P* container_of(M* ptr, const M P::*member)
+{
+    return (P*)((char*) ptr - (size_t) &(reinterpret_cast<P*>(0)->*member));
+}
 
 namespace Homa {
 namespace Util {
@@ -57,8 +58,6 @@ downCast(const Large& large)
 
 std::string demangle(const char* name);
 std::string hexDump(const void* buf, uint64_t bytes);
-std::string ipToString(uint32_t ip);
-uint32_t stringToIp(const char* ip);
 
 /**
  * This class is used to temporarily release lock in a safe fashion. Creating

src/Driver.cc

@@ -0,0 +1,38 @@
+/* Copyright (c) 2018-2019, Stanford University
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <Homa/Driver.h>
+
+#include "StringUtil.h"
+
+namespace Homa {
+
+std::string
+IpAddress::toString(IpAddress address)
+{
+    uint32_t ip = address.addr;
+    return StringUtil::format("%d.%d.%d.%d",
+        (ip >> 24) & 0xff, (ip >> 16) & 0xff, (ip >> 8) & 0xff, ip & 0xff);
+}
+
+IpAddress
+IpAddress::fromString(const char* addressStr)
+{
+    unsigned int b0, b1, b2, b3;
+    sscanf(addressStr, "%u.%u.%u.%u", &b0, &b1, &b2, &b3);
+    return IpAddress{(b0 << 24u) | (b1 << 16u) | (b2 << 8u) | b3};
+}
+
+}  // namespace Homa

src/Drivers/DPDK/DpdkDriver.cc

@@ -66,9 +66,10 @@ DpdkDriver::uncork()
 
 /// See Driver::receivePackets()
 uint32_t
-DpdkDriver::receivePackets(uint32_t maxPackets, Packet* receivedPackets[])
+DpdkDriver::receivePackets(uint32_t maxPackets, Packet* receivedPackets[],
+                           IpAddress sourceAddresses[])
 {
-    return pImpl->receivePackets(maxPackets, receivedPackets);
+    return pImpl->receivePackets(maxPackets, receivedPackets, sourceAddresses);
 }
 /// See Driver::releasePackets()
 void

src/Drivers/DPDK/DpdkDriverImpl.cc

@@ -51,7 +51,8 @@ const char* default_eal_argv[] = {"homa", NULL};
  *      Memory location in the mbuf where the packet data should be stored.
  */
 DpdkDriver::Impl::Packet::Packet(struct rte_mbuf* mbuf, void* data)
-    : base {.payload = data, .length = 0, .sourceIp = 0}
+    : base {.payload = data,
+            .length = 0}
     , bufType(MBUF)
     , bufRef()
 {
@@ -65,7 +66,8 @@ DpdkDriver::Impl::Packet::Packet(struct rte_mbuf* mbuf, void* data)
  *      Overflow buffer that holds this packet.
  */
 DpdkDriver::Impl::Packet::Packet(OverflowBuffer* overflowBuf)
-    : base {.payload = overflowBuf->data, .length = 0, .sourceIp = 0}
+    : base {.payload = overflowBuf->data,
+            .length = 0}
     , bufType(OVERFLOW_BUF)
     , bufRef()
 {
@@ -252,7 +254,8 @@ DpdkDriver::Impl::sendPacket(Driver::Packet* packet, IpAddress destination,
     vlanHdr->eth_proto = rte_cpu_to_be_16(EthPayloadType::HOMA);
 
     // Store our local IP address right before the payload.
-    *rte_pktmbuf_mtod_offset(mbuf, uint32_t*, PACKET_HDR_LEN - 4) = localIp;
+    *rte_pktmbuf_mtod_offset(mbuf, uint32_t*, PACKET_HDR_LEN - 4) =
+        (uint32_t)localIp;
 
     // In the normal case, we pre-allocate a pakcet's mbuf with enough
     // storage to hold the MAX_PAYLOAD_SIZE.  If the actual payload is
@@ -322,7 +325,8 @@ DpdkDriver::Impl::uncork()
 // See Driver::receivePackets()
 uint32_t
 DpdkDriver::Impl::receivePackets(uint32_t maxPackets,
-                                 Driver::Packet* receivedPackets[])
+                                 Driver::Packet* receivedPackets[],
+                                 IpAddress sourceAddresses[])
 {
     uint32_t numPacketsReceived = 0;
 
@@ -398,9 +402,10 @@ DpdkDriver::Impl::receivePackets(uint32_t maxPackets,
             packet = packetPool.construct(m, payload);
         }
         packet->base.length = length;
-        packet->base.sourceIp = srcIp;
 
-        receivedPackets[numPacketsReceived++] = &packet->base;
+        receivedPackets[numPacketsReceived] = &packet->base;
+        sourceAddresses[numPacketsReceived] = {srcIp};
+        ++numPacketsReceived;
     }
 
     return numPacketsReceived;
@@ -504,7 +509,7 @@ DpdkDriver::Impl::_init()
         int cols = sscanf(line.c_str(), "%s 0x%x 0x%x %99s %99s %99s\n",
                 ip, &type, &flags, hwa, mask, dev);
         if (cols != 6) continue;
-        arpTable.emplace(Homa::Util::stringToIp(ip), hwa);
+        arpTable.emplace(IpAddress::fromString(ip), hwa);
     }
 
     // Use ioctl to obtain the IP and MAC addresses of the network interface.
@@ -528,7 +533,7 @@ DpdkDriver::Impl::_init()
         throw DriverInitFailure(HERE_STR,
             StringUtil::format("Failed to obtain IP address: %s", error));
     }
-    localIp = be32toh(((struct sockaddr_in*) &ifr.ifr_addr)->sin_addr.s_addr);
+    localIp = {be32toh(((struct sockaddr_in*) &ifr.ifr_addr)->sin_addr.s_addr)};
 
     if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) {
         char* error = strerror(errno);
@@ -550,7 +555,7 @@ DpdkDriver::Impl::_init()
         }
     }
     NOTICE("Using interface %s, ip %s, mac %s, port %u",
-        ifname.c_str(), Homa::Util::ipToString(localIp).c_str(),
+        ifname.c_str(), IpAddress::toString(localIp).c_str(),
         localMac.toString().c_str(), port);
 
     std::string poolName = StringUtil::format("homa_mbuf_pool_%u", port);

src/Drivers/DPDK/DpdkDriverImpl.h

@@ -145,7 +145,8 @@ class DpdkDriver::Impl {
     void cork();
     void uncork();
     uint32_t receivePackets(uint32_t maxPackets,
-                            Driver::Packet* receivedPackets[]);
+                            Driver::Packet* receivedPackets[],
+                            IpAddress sourceAddresses[]);
     void releasePackets(Driver::Packet* packets[], uint16_t numPackets);
     int getHighestPacketPriority();
     uint32_t getMaxPayloadSize();
@@ -171,7 +172,7 @@ class DpdkDriver::Impl {
     uint16_t port;
 
     /// Address resolution table that translates IP addresses to MAC addresses.
-    std::unordered_map<IpAddress, MacAddress> arpTable;
+    std::unordered_map<IpAddress, MacAddress, IpAddress::Hasher> arpTable;
 
     /// Stores the IpAddress of the driver.
     IpAddress localIp;