Revert "dpif_netdev: Refactor dp_netdev_pmd_thread structure."

This reverts commit a807c15796ddc43ba1ffb2a6b0bd2ad4e2b73941.

Padding and aligning of dp_netdev_pmd_thread structure members
is useless, broken in a several ways and only greatly degrades
maintainability and extensibility of the structure.

Issues:

    1. It's not working because all the instances of struct
       dp_netdev_pmd_thread allocated only by usual malloc. All the
       memory is not aligned to cachelines -> structure almost never
       starts at aligned memory address. This means that any further
       paddings and alignments inside the structure are completely
       useless. Fo example:

       Breakpoint 1, pmd_thread_main
       (gdb) p pmd
       $49 = (struct dp_netdev_pmd_thread *) 0x1b1af20
       (gdb) p &pmd->cacheline1
       $51 = (OVS_CACHE_LINE_MARKER *) 0x1b1af60
       (gdb) p &pmd->cacheline0
       $52 = (OVS_CACHE_LINE_MARKER *) 0x1b1af20
       (gdb) p &pmd->flow_cache
       $53 = (struct emc_cache *) 0x1b1afe0

       All of the above addresses shifted from cacheline start by 32B.

       Can we fix it properly? NO.
       OVS currently doesn't have appropriate API to allocate aligned
       memory. The best candidate is 'xmalloc_cacheline()' but it
       clearly states that "The memory returned will not be at the
       start of a cache line, though, so don't assume such alignment".
       And also, this function will never return aligned memory on
       Windows or MacOS.

    2. CACHE_LINE_SIZE is not constant. Different architectures have
       different cache line sizes, but the code assumes that
       CACHE_LINE_SIZE is always equal to 64 bytes. All the structure
       members are grouped by 64 bytes and padded to CACHE_LINE_SIZE.
       This leads to a huge holes in a structures if CACHE_LINE_SIZE
       differs from 64. This is opposite to portability. If I want
       good performance of cmap I need to have CACHE_LINE_SIZE equal
       to the real cache line size, but I will have huge holes in the
       structures. If you'll take a look to struct rte_mbuf from DPDK
       you'll see that it uses 2 defines: RTE_CACHE_LINE_SIZE and
       RTE_CACHE_LINE_MIN_SIZE to avoid holes in mbuf structure.

    3. Sizes of system/libc defined types are not constant for all the
       systems. For example, sizeof(pthread_mutex_t) == 48 on my
       ARMv8 machine, but only 40 on x86. The difference could be
       much bigger on Windows or MacOS systems. But the code assumes
       that sizeof(struct ovs_mutex) is always 48 bytes. This may lead
       to broken alignment/big holes in case of padding/wrong comments
       about amount of free pad bytes.

    4. Sizes of the many fileds in structure depends on defines like
       DP_N_STATS, PMD_N_CYCLES, EM_FLOW_HASH_ENTRIES and so on.
       Any change in these defines or any change in any structure
       contained by thread should lead to the not so simple
       refactoring of the whole dp_netdev_pmd_thread structure. This
       greatly reduces maintainability and complicates development of
       a new features.

    5. There is no reason to align flow_cache member because it's
       too big and we usually access random entries by single thread
       only.

So, the padding/alignment only creates some visibility of performance
optimization but does nothing useful in reality. It only complicates
maintenance and adds huge holes for non-x86 architectures and non-Linux
systems. Performance improvement stated in a original commit message
should be random and not valuable. I see no performance difference.

Most of the above issues are also true for some other padded/aligned
structures like 'struct netdev_dpdk'. They will be treated separately.

CC: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
CC: Ben Pfaff <blp@ovn.org>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>
Signed-off-by: Ian Stokes <ian.stokes@intel.com>

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 4e5bdbebbd8b8c2b860a34ea41fed3b38920f747..43f6a785741254e418643b2a929ed3dc5d99e0fb 100644 (file)
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -547,31 +547,18 @@ struct tx_port {
  * actions in either case.
  * */
 struct dp_netdev_pmd_thread {
-    PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
-        struct dp_netdev *dp;
-        struct cmap_node node;          /* In 'dp->poll_threads'. */
-        pthread_cond_t cond;            /* For synchronizing pmd thread
-                                           reload. */
-    );
-
-    PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline1,
-        struct ovs_mutex cond_mutex;    /* Mutex for condition variable. */
-        pthread_t thread;
-        unsigned core_id;               /* CPU core id of this pmd thread. */
-        int numa_id;                    /* numa node id of this pmd thread. */
-    );
+    struct dp_netdev *dp;
+    struct ovs_refcount ref_cnt;    /* Every reference must be refcount'ed. */
+    struct cmap_node node;          /* In 'dp->poll_threads'. */
+
+    pthread_cond_t cond;            /* For synchronizing pmd thread reload. */
+    struct ovs_mutex cond_mutex;    /* Mutex for condition variable. */
 
     /* Per thread exact-match cache.  Note, the instance for cpu core
      * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
      * need to be protected by 'non_pmd_mutex'.  Every other instance
      * will only be accessed by its own pmd thread. */
-    OVS_ALIGNED_VAR(CACHE_LINE_SIZE) struct emc_cache flow_cache;
-    struct ovs_refcount ref_cnt;    /* Every reference must be refcount'ed. */
-
-    /* Queue id used by this pmd thread to send packets on all netdevs if
-     * XPS disabled for this netdev. All static_tx_qid's are unique and less
-     * than 'cmap_count(dp->poll_threads)'. */
-    uint32_t static_tx_qid;
+    struct emc_cache flow_cache;
 
     /* Flow-Table and classifiers
      *
@@ -580,77 +567,68 @@ struct dp_netdev_pmd_thread {
      * 'flow_mutex'.
      */
     struct ovs_mutex flow_mutex;
-    PADDED_MEMBERS(CACHE_LINE_SIZE,
-        struct cmap flow_table OVS_GUARDED; /* Flow table. */
-
-        /* One classifier per in_port polled by the pmd */
-        struct cmap classifiers;
-        /* Periodically sort subtable vectors according to hit frequencies */
-        long long int next_optimization;
-        /* End of the next time interval for which processing cycles
-           are stored for each polled rxq. */
-        long long int rxq_next_cycle_store;
-
-        /* Cycles counters */
-        struct dp_netdev_pmd_cycles cycles;
-
-        /* Used to count cycles. See 'cycles_counter_end()'. */
-        unsigned long long last_cycles;
-        struct latch exit_latch;        /* For terminating the pmd thread. */
-    );
-
-    PADDED_MEMBERS(CACHE_LINE_SIZE,
-        /* Statistics. */
-        struct dp_netdev_pmd_stats stats;
-
-        struct seq *reload_seq;
-        uint64_t last_reload_seq;
-        atomic_bool reload;             /* Do we need to reload ports? */
-        bool isolated;
-
-        /* Set to true if the pmd thread needs to be reloaded. */
-        bool need_reload;
-        /* 5 pad bytes. */
-    );
-
-    PADDED_MEMBERS(CACHE_LINE_SIZE,
-        struct ovs_mutex port_mutex;    /* Mutex for 'poll_list'
-                                           and 'tx_ports'. */
-        /* 16 pad bytes. */
-    );
-    PADDED_MEMBERS(CACHE_LINE_SIZE,
-        /* List of rx queues to poll. */
-        struct hmap poll_list OVS_GUARDED;
-        /* Map of 'tx_port's used for transmission.  Written by the main
-         * thread, read by the pmd thread. */
-        struct hmap tx_ports OVS_GUARDED;
-    );
-    PADDED_MEMBERS(CACHE_LINE_SIZE,
-        /* These are thread-local copies of 'tx_ports'.  One contains only
-         * tunnel ports (that support push_tunnel/pop_tunnel), the other
-         * contains ports with at least one txq (that support send).
-         * A port can be in both.
-         *
-         * There are two separate maps to make sure that we don't try to
-         * execute OUTPUT on a device which has 0 txqs or PUSH/POP on a
-         * non-tunnel device.
-         *
-         * The instances for cpu core NON_PMD_CORE_ID can be accessed by
-         * multiple threads and thusly need to be protected by 'non_pmd_mutex'.
-         * Every other instance will only be accessed by its own pmd thread. */
-        struct hmap tnl_port_cache;
-        struct hmap send_port_cache;
-    );
-
-    PADDED_MEMBERS(CACHE_LINE_SIZE,
-        /* Only a pmd thread can write on its own 'cycles' and 'stats'.
-         * The main thread keeps 'stats_zero' and 'cycles_zero' as base
-         * values and subtracts them from 'stats' and 'cycles' before
-         * reporting to the user */
-        unsigned long long stats_zero[DP_N_STATS];
-        uint64_t cycles_zero[PMD_N_CYCLES];
-        /* 8 pad bytes. */
-    );
+    struct cmap flow_table OVS_GUARDED; /* Flow table. */
+
+    /* One classifier per in_port polled by the pmd */
+    struct cmap classifiers;
+    /* Periodically sort subtable vectors according to hit frequencies */
+    long long int next_optimization;
+    /* End of the next time interval for which processing cycles
+       are stored for each polled rxq. */
+    long long int rxq_next_cycle_store;
+
+    /* Statistics. */
+    struct dp_netdev_pmd_stats stats;
+
+    /* Cycles counters */
+    struct dp_netdev_pmd_cycles cycles;
+
+    /* Used to count cicles. See 'cycles_counter_end()' */
+    unsigned long long last_cycles;
+
+    struct latch exit_latch;        /* For terminating the pmd thread. */
+    struct seq *reload_seq;
+    uint64_t last_reload_seq;
+    atomic_bool reload;             /* Do we need to reload ports? */
+    pthread_t thread;
+    unsigned core_id;               /* CPU core id of this pmd thread. */
+    int numa_id;                    /* numa node id of this pmd thread. */
+    bool isolated;
+
+    /* Queue id used by this pmd thread to send packets on all netdevs if
+     * XPS disabled for this netdev. All static_tx_qid's are unique and less
+     * than 'cmap_count(dp->poll_threads)'. */
+    uint32_t static_tx_qid;
+
+    struct ovs_mutex port_mutex;    /* Mutex for 'poll_list' and 'tx_ports'. */
+    /* List of rx queues to poll. */
+    struct hmap poll_list OVS_GUARDED;
+    /* Map of 'tx_port's used for transmission.  Written by the main thread,
+     * read by the pmd thread. */
+    struct hmap tx_ports OVS_GUARDED;
+
+    /* These are thread-local copies of 'tx_ports'.  One contains only tunnel
+     * ports (that support push_tunnel/pop_tunnel), the other contains ports
+     * with at least one txq (that support send).  A port can be in both.
+     *
+     * There are two separate maps to make sure that we don't try to execute
+     * OUTPUT on a device which has 0 txqs or PUSH/POP on a non-tunnel device.
+     *
+     * The instances for cpu core NON_PMD_CORE_ID can be accessed by multiple
+     * threads, and thusly need to be protected by 'non_pmd_mutex'.  Every
+     * other instance will only be accessed by its own pmd thread. */
+    struct hmap tnl_port_cache;
+    struct hmap send_port_cache;
+
+    /* Only a pmd thread can write on its own 'cycles' and 'stats'.
+     * The main thread keeps 'stats_zero' and 'cycles_zero' as base
+     * values and subtracts them from 'stats' and 'cycles' before
+     * reporting to the user */
+    unsigned long long stats_zero[DP_N_STATS];
+    uint64_t cycles_zero[PMD_N_CYCLES];
+
+    /* Set to true if the pmd thread needs to be reloaded. */
+    bool need_reload;
 };
 
 /* Interface to netdev-based datapath. */