[ovs-dev] [PATCH] ovs-thread: Avoid huge alignment on a base spinlock structure.

[Ilya Maximets]

Marking the structure as 64 bytes aligned forces compiler to produce
big holes in the containing structures in order to fulfill this
requirement.  Also, any structure that contains this one as a member
automatically inherits this huge alignment making resulted memory
layout not efficient.  For example, 'struct umem_pool' currently
uses 3 full cache lines (192 bytes) with only 32 bytes of actual data:

  struct umem_pool {
    int                        index;                /*  0   4 */
    unsigned int               size;                 /*  4   4 */

    /* XXX 56 bytes hole, try to pack */

    /* --- cacheline 1 boundary (64 bytes) --- */
    struct ovs_spin lock __attribute__((__aligned__(64))); /* 64  64 */

    /* XXX last struct has 48 bytes of padding */

    /* --- cacheline 2 boundary (128 bytes) --- */
    void * *                   array;                /* 128  8 */

    /* size: 192, cachelines: 3, members: 4 */
    /* sum members: 80, holes: 1, sum holes: 56 */
    /* padding: 56 */
    /* paddings: 1, sum paddings: 48 */
    /* forced alignments: 1, forced holes: 1, sum forced holes: 56 */
  } __attribute__((__aligned__(64)));

Actual alignment of a spin lock is required only for Tx queue locks
inside netdev-afxdp to avoid false sharing, in all other cases
alignment only produces inefficient memory usage.

Also, CACHE_LINE_SIZE macro should be used instead of 64 as different
platforms may have different cache line sizes.

Using PADDED_MEMBERS to avoid alignment inheritance.

CC: William Tu <u9012063 at gmail.com>
Fixes: ae36d63d7e3c ("ovs-thread: Make struct spin lock cache aligned.")
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
---
 include/openvswitch/thread.h |  2 +-
 lib/netdev-afxdp.c           | 20 ++++++++++++++------
 lib/netdev-afxdp.h           | 13 +++++++------
 lib/netdev-linux-private.h   |  2 +-
 4 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/include/openvswitch/thread.h b/include/openvswitch/thread.h
index 5053cb309..acc822904 100644
--- a/include/openvswitch/thread.h
+++ b/include/openvswitch/thread.h
@@ -34,7 +34,7 @@ struct OVS_LOCKABLE ovs_mutex {
 };
 
 #ifdef HAVE_PTHREAD_SPIN_LOCK
-OVS_ALIGNED_STRUCT(64, OVS_LOCKABLE ovs_spin) {
+struct OVS_LOCKABLE ovs_spin {
     pthread_spinlock_t lock;
     const char *where;          /* NULL if and only if uninitialized. */
 };
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index ca2dfd005..c59a671e7 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -40,6 +40,7 @@
 #include "openvswitch/compiler.h"
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/list.h"
+#include "openvswitch/thread.h"
 #include "openvswitch/vlog.h"
 #include "packets.h"
 #include "socket-util.h"
@@ -158,6 +159,13 @@ struct xsk_socket_info {
     atomic_uint64_t tx_dropped;
 };
 
+struct netdev_afxdp_tx_lock {
+    /* Padding to make netdev_afxdp_tx_lock exactly one cache line long. */
+    PADDED_MEMBERS(CACHE_LINE_SIZE,
+        struct ovs_spin lock;
+    );
+};
+
 #ifdef HAVE_XDP_NEED_WAKEUP
 static inline void
 xsk_rx_wakeup_if_needed(struct xsk_umem_info *umem,
@@ -512,10 +520,10 @@ xsk_configure_all(struct netdev *netdev)
     }
 
     n_txq = netdev_n_txq(netdev);
-    dev->tx_locks = xcalloc(n_txq, sizeof *dev->tx_locks);
+    dev->tx_locks = xzalloc_cacheline(n_txq * sizeof *dev->tx_locks);
 
     for (i = 0; i < n_txq; i++) {
-        ovs_spin_init(&dev->tx_locks[i]);
+        ovs_spin_init(&dev->tx_locks[i].lock);
     }
 
     return 0;
@@ -577,9 +585,9 @@ xsk_destroy_all(struct netdev *netdev)
 
     if (dev->tx_locks) {
         for (i = 0; i < netdev_n_txq(netdev); i++) {
-            ovs_spin_destroy(&dev->tx_locks[i]);
+            ovs_spin_destroy(&dev->tx_locks[i].lock);
         }
-        free(dev->tx_locks);
+        free_cacheline(dev->tx_locks);
         dev->tx_locks = NULL;
     }
 }
@@ -1076,9 +1084,9 @@ netdev_afxdp_batch_send(struct netdev *netdev, int qid,
         dev = netdev_linux_cast(netdev);
         qid = qid % netdev_n_txq(netdev);
 
-        ovs_spin_lock(&dev->tx_locks[qid]);
+        ovs_spin_lock(&dev->tx_locks[qid].lock);
         ret = __netdev_afxdp_batch_send(netdev, qid, batch);
-        ovs_spin_unlock(&dev->tx_locks[qid]);
+        ovs_spin_unlock(&dev->tx_locks[qid].lock);
     } else {
         ret = __netdev_afxdp_batch_send(netdev, qid, batch);
     }
diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h
index 8188bc669..ae6971efd 100644
--- a/lib/netdev-afxdp.h
+++ b/lib/netdev-afxdp.h
@@ -34,15 +34,16 @@ enum afxdp_mode {
     OVS_AF_XDP_MODE_MAX,
 };
 
-struct netdev;
-struct xsk_socket_info;
-struct xdp_umem;
-struct dp_packet_batch;
-struct smap;
 struct dp_packet;
+struct dp_packet_batch;
+struct netdev;
+struct netdev_afxdp_tx_lock;
+struct netdev_custom_stats;
 struct netdev_rxq;
 struct netdev_stats;
-struct netdev_custom_stats;
+struct smap;
+struct xdp_umem;
+struct xsk_socket_info;
 
 int netdev_afxdp_rxq_construct(struct netdev_rxq *rxq_);
 void netdev_afxdp_rxq_destruct(struct netdev_rxq *rxq_);
diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
index 8873caa9d..f08159aa7 100644
--- a/lib/netdev-linux-private.h
+++ b/lib/netdev-linux-private.h
@@ -108,7 +108,7 @@ struct netdev_linux {
     bool use_need_wakeup;
     bool requested_need_wakeup;
 
-    struct ovs_spin *tx_locks;  /* spin lock array for TX queues. */
+    struct netdev_afxdp_tx_lock *tx_locks;  /* Array of locks for TX queues. */
 #endif
 };
 
-- 
2.17.1