pinctrl: Convert the put_ARP/ND methods to be lockless.

The put_ARP/ND were bound to pinctrl lock to prevent data races as
they have used shared hmap to store the data that were later on
added into SB. This is problematic in situations when main thread
holds the lock and we have to process any put_ARP/ND packet. That
contention would block the pinctrl thread from processing the
incoming packets. The same can happen from the opposite side, pinctrl
could be holding a lock, delaying main thread from processing other
updates.

To prevent that use the hmap only by pinctrl thread, this will ensure
that we wait properly for the multicast timeout to expire and at the
same time to do the deduplication. Once the entry is ready it will be
shared by thread safe ring buffer and consumed by main thread.

Signed-off-by: Ales Musil <amusil@redhat.com>

Author: almusil

controller/pinctrl.c

@@ -65,6 +65,7 @@
 #include "ovn-sb-idl.h"
 #include "ovn-dns.h"
 #include "garp_rarp.h"
+#include "spsc-ring.h"
 
 VLOG_DEFINE_THIS_MODULE(pinctrl);
 
@@ -83,24 +84,31 @@ VLOG_DEFINE_THIS_MODULE(pinctrl);
  * and pinctrl_run().
  *
  *
- *   - put_arp/put_nd - These actions stores the IPv4/IPv6 and MAC addresses
+ *   - put_arp/put_nd - These actions store the IPv4/IPv6 and MAC addresses
  *                      in the 'MAC_Binding' table.
  *                      The function 'pinctrl_handle_put_mac_binding()' (which
- *                      is called with in the pinctrl_handler thread), stores
+ *                      is called within the pinctrl_handler thread), stores
  *                      the IPv4/IPv6 and MAC addresses in the
- *                      hmap - put_mac_bindings.
+ *                      hmap - 'wait_mac_bindings'.  This hmap is owned
+ *                      exclusively by the pinctrl_handler thread and does
+ *                      not require pinctrl_mutex.
  *
- *                      pinctrl_run(), reads these mac bindings from the hmap
- *                      'put_mac_bindings' and writes to the 'MAC_Binding'
- *                      table in the Southbound DB.
+ *                      When the multicast delay timer expires, entries are
+ *                      pushed into a lock-free SPSC ring buffer
+ *                      ('mac_bindings_ring') by
+ *                      wait_mac_bindings_prepare_ready().
+ *
+ *                      pinctrl_run() drains the ring in
+ *                      run_put_mac_bindings() and writes the mac bindings
+ *                      to the 'MAC_Binding' table in the Southbound DB.
  *
  *   - arp/nd_ns      - These actions generate an ARP/IPv6 Neighbor solicit
  *                      requests. The original packets are buffered and
  *                      injected back when put_arp/put_nd resolves
  *                      corresponding ARP/IPv6 Neighbor solicit requests.
- *                      When pinctrl_run(), writes the mac bindings from the
- *                      'put_mac_bindings' hmap to the MAC_Binding table in
- *                      SB DB, run_buffered_binding will add the buffered
+ *                      When pinctrl_run() writes the mac bindings from the
+ *                      ring buffer to the MAC_Binding table in SB DB,
+ *                      run_buffered_binding will add the buffered
  *                      packets to buffered_mac_bindings and notify
  *                      pinctrl_handler.
  *
@@ -197,17 +205,15 @@ run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table,
 
 static void pinctrl_handle_put_mac_binding(const struct flow *md,
                                            const struct flow *headers,
-                                           bool is_arp)
-    OVS_REQUIRES(pinctrl_mutex);
-static void init_put_mac_bindings(void);
-static void destroy_put_mac_bindings(void);
+                                           bool is_arp);
+static void init_mac_bindings(void);
+static void destroy_mac_bindings(void);
 static void run_put_mac_bindings(
     struct ovsdb_idl_txn *ovnsb_idl_txn,
     struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     struct ovsdb_idl_index *sbrec_port_binding_by_key,
-    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
-    OVS_REQUIRES(pinctrl_mutex);
-static void wait_put_mac_bindings(void);
+    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip);
+static void wait_mac_bindings_prepare_ready(void);
 static void send_mac_binding_buffered_pkts(struct rconn *swconn);
 
 static void pinctrl_activation_strategy_handler(const struct match *md);
@@ -556,7 +562,7 @@ controller_event_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
 void
 pinctrl_init(void)
 {
-    init_put_mac_bindings();
+    init_mac_bindings();
     init_send_arps_nds();
     init_ipv6_ras();
     init_ipv6_prefixd();
@@ -3776,10 +3782,8 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
         break;
 
     case ACTION_OPCODE_PUT_ARP:
-        ovs_mutex_lock(&pinctrl_mutex);
         pinctrl_handle_put_mac_binding(&pin.flow_metadata.flow, &headers,
                                        true);
-        ovs_mutex_unlock(&pinctrl_mutex);
         break;
 
     case ACTION_OPCODE_DHCP_RELAY_REQ_CHK:
@@ -3808,10 +3812,8 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
         break;
 
     case ACTION_OPCODE_PUT_ND:
-        ovs_mutex_lock(&pinctrl_mutex);
         pinctrl_handle_put_mac_binding(&pin.flow_metadata.flow, &headers,
                                        false);
-        ovs_mutex_unlock(&pinctrl_mutex);
         break;
 
     case ACTION_OPCODE_PUT_FDB:
@@ -4100,6 +4102,8 @@ pinctrl_handler(void *arg_)
             poll_timer_wait(5);
         }
 
+        wait_mac_bindings_prepare_ready();
+
         rconn_run_wait(swconn);
         rconn_recv_wait(swconn);
         if (rconn_is_connected(swconn)) {
@@ -4724,7 +4728,6 @@ pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn)
 {
     ovs_mutex_lock(&pinctrl_mutex);
     if (ovnsb_idl_txn) {
-        wait_put_mac_bindings();
         wait_controller_event();
         wait_put_vport_bindings();
         wait_put_fdbs();
@@ -4757,7 +4760,7 @@ pinctrl_destroy(void)
     destroy_buffered_packets_map();
     destroy_activated_ports();
     event_table_destroy();
-    destroy_put_mac_bindings();
+    destroy_mac_bindings();
     destroy_put_vport_bindings();
     ip_mcast_snoop_destroy();
     destroy_svc_monitors();
@@ -4782,36 +4785,37 @@ pinctrl_destroy(void)
 
 #define MAX_MAC_BINDING_DELAY_MSEC 50
 #define MAX_FDB_DELAY_MSEC         50
-#define MAX_MAC_BINDINGS           1000
+#define MAX_MAC_BINDINGS           1024
+
 
-/* Contains "struct mac_binding"s. */
-static struct hmap put_mac_bindings;
+/* Contains "struct mac_binding"s. This is used by pinctrl thread only.  */
+static struct hmap wait_mac_bindings;
+/* Contains "struct mac_binding_data". This is populated by pinctrl thread
+ * and consumed by main thread. */
+static struct spsc_ring mac_bindings_ring;
 
 static void
-init_put_mac_bindings(void)
+init_mac_bindings(void)
 {
-    hmap_init(&put_mac_bindings);
+    hmap_init(&wait_mac_bindings);
+    spsc_ring_init(&mac_bindings_ring, MAX_MAC_BINDINGS,
+                   sizeof (struct mac_binding_data));
 }
 
 static void
-destroy_put_mac_bindings(void)
+destroy_mac_bindings(void)
 {
-    mac_bindings_clear(&put_mac_bindings);
-    hmap_destroy(&put_mac_bindings);
+    mac_bindings_clear(&wait_mac_bindings);
+    hmap_destroy(&wait_mac_bindings);
+    spsc_ring_destroy(&mac_bindings_ring);
 }
 
 /* Called with in the pinctrl_handler thread context. */
 static void
 pinctrl_handle_put_mac_binding(const struct flow *md,
                                const struct flow *headers,
                                bool is_arp)
-    OVS_REQUIRES(pinctrl_mutex)
 {
-    if (hmap_count(&put_mac_bindings) >= MAX_MAC_BINDINGS) {
-        COVERAGE_INC(pinctrl_drop_put_mac_binding);
-        return;
-    }
-
     struct mac_binding_data mb_data = (struct mac_binding_data) {
             .dp_key =  ntohll(md->metadata),
             .port_key =  md->regs[MFF_LOG_INPORT - MFF_REG0],
@@ -4831,12 +4835,7 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
                      ? random_range(MAX_MAC_BINDING_DELAY_MSEC) + 1
                      : 0;
     long long timestamp = time_msec() + delay;
-    mac_binding_add(&put_mac_bindings, mb_data, NULL, timestamp);
-
-    /* We can send the buffered packet once the main ovn-controller
-     * thread calls pinctrl_run() and it writes the mac_bindings stored
-     * in 'put_mac_bindings' hmap into the Southbound MAC_Binding table. */
-    notify_pinctrl_main();
+    mac_binding_add(&wait_mac_bindings, mb_data, NULL, timestamp);
 }
 
 /* Called with in the pinctrl_handler thread context. */
@@ -4864,29 +4863,29 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
                     struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                     struct ovsdb_idl_index *sbrec_port_binding_by_key,
                     struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
-                    const struct mac_binding *mb)
+                    const struct mac_binding_data *mb_data)
 {
     /* Convert logical datapath and logical port key into lport. */
     const struct sbrec_port_binding *pb = lport_lookup_by_key(
         sbrec_datapath_binding_by_key, sbrec_port_binding_by_key,
-        mb->data.dp_key, mb->data.port_key);
+        mb_data->dp_key, mb_data->port_key);
     if (!pb) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
         VLOG_WARN_RL(&rl, "unknown logical port with datapath %"PRIu32" "
-                     "and port %"PRIu32, mb->data.dp_key, mb->data.port_key);
+                     "and port %"PRIu32, mb_data->dp_key, mb_data->port_key);
         return;
     }
 
     /* Convert ethernet argument to string form for database. */
     char mac_string[ETH_ADDR_STRLEN + 1];
     snprintf(mac_string, sizeof mac_string,
-             ETH_ADDR_FMT, ETH_ADDR_ARGS(mb->data.mac));
+             ETH_ADDR_FMT, ETH_ADDR_ARGS(mb_data->mac));
 
     struct ds ip_s = DS_EMPTY_INITIALIZER;
-    ipv6_format_mapped(&mb->data.ip, &ip_s);
+    ipv6_format_mapped(&mb_data->ip, &ip_s);
     mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
-                          pb->logical_port, pb->datapath, mb->data.mac,
+                          pb->logical_port, pb->datapath, mb_data->mac,
                           ds_cstr(&ip_s), false);
     ds_destroy(&ip_s);
 }
@@ -4898,23 +4897,16 @@ run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn,
                      struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                      struct ovsdb_idl_index *sbrec_port_binding_by_key,
                      struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
-    OVS_REQUIRES(pinctrl_mutex)
 {
     if (!ovnsb_idl_txn) {
         return;
     }
 
-    long long now = time_msec();
-
-    struct mac_binding *mb;
-    HMAP_FOR_EACH_SAFE (mb, hmap_node, &put_mac_bindings) {
-        if (now >= mb->timestamp) {
-            run_put_mac_binding(ovnsb_idl_txn,
-                                sbrec_datapath_binding_by_key,
-                                sbrec_port_binding_by_key,
-                                sbrec_mac_binding_by_lport_ip, mb);
-            mac_binding_remove(&put_mac_bindings, mb);
-        }
+    struct mac_binding_data mb_data;
+    SPSC_RING_FOR_EACH_POP (&mac_bindings_ring, mb_data) {
+        run_put_mac_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
+                            sbrec_port_binding_by_key,
+                            sbrec_mac_binding_by_lport_ip, &mb_data);
     }
 }
 
@@ -4985,12 +4977,29 @@ run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table,
 }
 
 static void
-wait_put_mac_bindings(void)
-    OVS_REQUIRES(pinctrl_mutex)
+wait_mac_bindings_prepare_ready(void)
 {
+    long long now = time_msec();
+    bool notify = false;
+
     struct mac_binding *mb;
-    HMAP_FOR_EACH (mb, hmap_node, &put_mac_bindings) {
-        poll_timer_wait_until(mb->timestamp);
+    HMAP_FOR_EACH_SAFE (mb, hmap_node, &wait_mac_bindings) {
+        if (now < mb->timestamp) {
+            poll_timer_wait_until(mb->timestamp);
+            continue;
+        }
+
+        if (spsc_ring_push(&mac_bindings_ring, &mb->data)) {
+            notify = true;
+        } else {
+            COVERAGE_INC(pinctrl_drop_put_mac_binding);
+        }
+
+        mac_binding_remove(&wait_mac_bindings, mb);
+    }
+
+    if (notify) {
+        notify_pinctrl_main();
     }
 }