[ovs-dev] OVN: fix pinctrl ip buffering for gw router port
diff mbox series

Message ID cd4988e0ff24b6bf49f90b6b2d7eea36fe7d31e8.1557233465.git.lorenzo.bianconi@redhat.com
State New
Headers show
Series
  • [ovs-dev] OVN: fix pinctrl ip buffering for gw router port
Related show

Commit Message

Lorenzo Bianconi May 7, 2019, 1:08 p.m. UTC
Use sb mac binding table to trigger ip buffer dequeueing instead of
the APR/ND packet reception since the ARP reply can be managed on a
different chassis if a gw router port is scheduled on a different
node

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 ovn/controller/pinctrl.c | 113 +++++++++++++++++++++++----------------
 tests/ovn.at             |  11 ++--
 2 files changed, 72 insertions(+), 52 deletions(-)

Comments

Ben Pfaff May 8, 2019, 12:18 a.m. UTC | #1
On Tue, May 07, 2019 at 03:08:52PM +0200, Lorenzo Bianconi wrote:
> Use sb mac binding table to trigger ip buffer dequeueing instead of
> the APR/ND packet reception since the ARP reply can be managed on a
> different chassis if a gw router port is scheduled on a different
> node
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Thanks!

buffered_mac_bindings doesn't seem to really be used as a hash table:
nothing ever searches it.  Should it be a linked list?  Or maybe it's
just an hmap because destroy_buffered_packets() expects that.  Not a big
deal, just a surprise.

I don't think that really matters, and this looked good to me otherwise,
so I applied it to master.
Lorenzo Bianconi May 8, 2019, 12:59 p.m. UTC | #2
> On Tue, May 07, 2019 at 03:08:52PM +0200, Lorenzo Bianconi wrote:
> > Use sb mac binding table to trigger ip buffer dequeueing instead of
> > the APR/ND packet reception since the ARP reply can be managed on a
> > different chassis if a gw router port is scheduled on a different
> > node
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Thanks!
> 
> buffered_mac_bindings doesn't seem to really be used as a hash table:
> nothing ever searches it.  Should it be a linked list?  Or maybe it's
> just an hmap because destroy_buffered_packets() expects that.  Not a big
> deal, just a surprise.

Hi Ben,

Thx for the fast review :)
Yes, you right. I just maintained the same code architecture. I will post a
patch to convert buffered_mac_bindings to a linked list

Regards,
Lorenzo

> 
> I don't think that really matters, and this looked good to me otherwise,
> so I applied it to master.
Ben Pfaff May 8, 2019, 4:54 p.m. UTC | #3
On Wed, May 08, 2019 at 02:59:32PM +0200, Lorenzo Bianconi wrote:
> > On Tue, May 07, 2019 at 03:08:52PM +0200, Lorenzo Bianconi wrote:
> > > Use sb mac binding table to trigger ip buffer dequeueing instead of
> > > the APR/ND packet reception since the ARP reply can be managed on a
> > > different chassis if a gw router port is scheduled on a different
> > > node
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > 
> > Thanks!
> > 
> > buffered_mac_bindings doesn't seem to really be used as a hash table:
> > nothing ever searches it.  Should it be a linked list?  Or maybe it's
> > just an hmap because destroy_buffered_packets() expects that.  Not a big
> > deal, just a surprise.
> 
> Hi Ben,
> 
> Thx for the fast review :)
> Yes, you right. I just maintained the same code architecture. I will post a
> patch to convert buffered_mac_bindings to a linked list

OK.  I'll take a look.  Maybe it does not actually make the code better;
I'll see.

Patch
diff mbox series

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 2ae79cfd4..4f1991d29 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -149,6 +149,11 @@  static struct pinctrl pinctrl;
 
 static void init_buffered_packets_map(void);
 static void destroy_buffered_packets_map(void);
+static void
+run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
+                     struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+                     const struct hmap *local_datapaths)
+    OVS_REQUIRES(pinctrl_mutex);
 
 static void pinctrl_handle_put_mac_binding(const struct flow *md,
                                            const struct flow *headers,
@@ -164,8 +169,6 @@  static void run_put_mac_bindings(
     OVS_REQUIRES(pinctrl_mutex);
 static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
 static void flush_put_mac_bindings(void);
-static void buffer_put_mac_bindings(void);
-static void destroy_buffered_mac_bindings(void);
 static void send_mac_binding_buffered_pkts(struct rconn *swconn)
     OVS_REQUIRES(pinctrl_mutex);
 
@@ -321,6 +324,7 @@  struct buffered_packets {
 
     /* key */
     struct in6_addr ip;
+    struct eth_addr ea;
 
     long long int timestamp;
 
@@ -329,15 +333,17 @@  struct buffered_packets {
 };
 
 static struct hmap buffered_packets_map;
+static struct hmap buffered_mac_bindings;
 
 static void
 init_buffered_packets_map(void)
 {
     hmap_init(&buffered_packets_map);
+    hmap_init(&buffered_mac_bindings);
 }
 
 static void
-destroy_buffered_packets(struct buffered_packets *bp)
+destroy_buffered_packets(struct buffered_packets *bp, struct hmap *map)
 {
     struct buffer_info *bi;
 
@@ -348,7 +354,7 @@  destroy_buffered_packets(struct buffered_packets *bp)
 
         bp->head = (bp->head + 1) % BUFFER_QUEUE_DEPTH;
     }
-    hmap_remove(&buffered_packets_map, &bp->hmap_node);
+    hmap_remove(map, &bp->hmap_node);
     free(bp);
 }
 
@@ -357,9 +363,14 @@  destroy_buffered_packets_map(void)
 {
     struct buffered_packets *bp, *next;
     HMAP_FOR_EACH_SAFE (bp, next, hmap_node, &buffered_packets_map) {
-        destroy_buffered_packets(bp);
+        destroy_buffered_packets(bp, &buffered_packets_map);
     }
     hmap_destroy(&buffered_packets_map);
+
+    HMAP_FOR_EACH_SAFE (bp, next, hmap_node, &buffered_mac_bindings) {
+        destroy_buffered_packets(bp, &buffered_mac_bindings);
+    }
+    hmap_destroy(&buffered_mac_bindings);
 }
 
 static void
@@ -426,7 +437,7 @@  buffered_packets_map_gc(void)
 
     HMAP_FOR_EACH_SAFE (cur_qp, next_qp, hmap_node, &buffered_packets_map) {
         if (now > cur_qp->timestamp + BUFFER_MAP_TIMEOUT) {
-            destroy_buffered_packets(cur_qp);
+            destroy_buffered_packets(cur_qp, &buffered_packets_map);
         }
     }
 }
@@ -1851,7 +1862,6 @@  pinctrl_handler(void *arg_)
             }
         }
 
-        buffered_packets_map_gc();
         rconn_run_wait(swconn);
         rconn_recv_wait(swconn);
         send_garp_wait(send_garp_time);
@@ -1903,6 +1913,9 @@  pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
     prepare_ipv6_ras(sbrec_port_binding_by_datapath,
                      sbrec_port_binding_by_name, local_datapaths);
     sync_dns_cache(dns_table);
+    run_buffered_binding(sbrec_port_binding_by_datapath,
+                         sbrec_mac_binding_by_lport_ip,
+                         local_datapaths);
     ovs_mutex_unlock(&pinctrl_mutex);
 }
 
@@ -2249,7 +2262,6 @@  pinctrl_destroy(void)
     destroy_ipv6_ras();
     destroy_buffered_packets_map();
     destroy_put_mac_bindings();
-    destroy_buffered_mac_bindings();
     destroy_dns_cache();
     seq_destroy(pinctrl_main_seq);
     seq_destroy(pinctrl_handler_seq);
@@ -2283,13 +2295,11 @@  struct put_mac_binding {
 
 /* Contains "struct put_mac_binding"s. */
 static struct hmap put_mac_bindings;
-static struct hmap buffered_mac_bindings;
 
 static void
 init_put_mac_bindings(void)
 {
     hmap_init(&put_mac_bindings);
-    hmap_init(&buffered_mac_bindings);
 }
 
 static void
@@ -2299,17 +2309,6 @@  destroy_put_mac_bindings(void)
     hmap_destroy(&put_mac_bindings);
 }
 
-static void
-destroy_buffered_mac_bindings(void)
-{
-    struct put_mac_binding *pmb;
-    HMAP_FOR_EACH_POP (pmb, hmap_node, &buffered_mac_bindings) {
-       free(pmb);
-    }
-
-    hmap_destroy(&buffered_mac_bindings);
-}
-
 static struct put_mac_binding *
 pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t port_key,
                              const struct in6_addr *ip_key, uint32_t hash)
@@ -2372,17 +2371,10 @@  static void
 send_mac_binding_buffered_pkts(struct rconn *swconn)
     OVS_REQUIRES(pinctrl_mutex)
 {
-    struct put_mac_binding *pmb;
     struct buffered_packets *bp;
-    HMAP_FOR_EACH_POP (pmb, hmap_node, &buffered_mac_bindings) {
-        uint32_t bhash = hash_bytes(&pmb->ip_key, sizeof pmb->ip_key, 0);
-
-        bp = pinctrl_find_buffered_packets(&pmb->ip_key, bhash);
-        if (bp) {
-            buffered_send_packets(swconn, bp, &pmb->mac);
-        }
-
-        free(pmb);
+    HMAP_FOR_EACH_POP (bp, hmap_node, &buffered_mac_bindings) {
+        buffered_send_packets(swconn, bp, &bp->ea);
+        free(bp);
     }
 }
 
@@ -2472,12 +2464,50 @@  run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn,
                             sbrec_mac_binding_by_lport_ip,
                             pmb);
     }
+}
+
+static void
+run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
+                     struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+                     const struct hmap *local_datapaths)
+    OVS_REQUIRES(pinctrl_mutex)
+{
+    const struct local_datapath *ld;
+    bool notify = false;
+
+    HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
+        struct sbrec_port_binding *target = sbrec_port_binding_index_init_row(
+                sbrec_port_binding_by_datapath);
+        sbrec_port_binding_index_set_datapath(target, ld->datapath);
+
+        const struct sbrec_port_binding *pb;
+        SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
+                                           sbrec_port_binding_by_datapath) {
+            struct buffered_packets *cur_qp, *next_qp;
+            HMAP_FOR_EACH_SAFE (cur_qp, next_qp, hmap_node,
+                                &buffered_packets_map) {
+                struct ds ip_s = DS_EMPTY_INITIALIZER;
+                ipv6_format_mapped(&cur_qp->ip, &ip_s);
+                const struct sbrec_mac_binding *b = mac_binding_lookup(
+                        sbrec_mac_binding_by_lport_ip, pb->logical_port,
+                        ds_cstr(&ip_s));
+                if (b && ovs_scan(b->mac, ETH_ADDR_SCAN_FMT,
+                                  ETH_ADDR_SCAN_ARGS(cur_qp->ea))) {
+                    uint32_t hash = hash_bytes(&cur_qp->ip,
+                                               sizeof cur_qp->ip, 0);
+                    hmap_remove(&buffered_packets_map, &cur_qp->hmap_node);
+                    hmap_insert(&buffered_mac_bindings, &cur_qp->hmap_node,
+                                hash);
+                    notify = true;
+                }
+                ds_destroy(&ip_s);
+            }
+        }
+        sbrec_port_binding_index_destroy_row(target);
+    }
+    buffered_packets_map_gc();
 
-    /* Move the mac bindings from 'put_mac_bindings' hmap to
-     * 'buffered_mac_bindings' and notify the pinctrl_handler.
-     * pinctrl_handler will reinject the buffered packets. */
-    if (!hmap_is_empty(&put_mac_bindings)) {
-        buffer_put_mac_bindings();
+    if (notify) {
         notify_pinctrl_handler();
     }
 }
@@ -2490,17 +2520,6 @@  wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn)
     }
 }
 
-static void
-buffer_put_mac_bindings(void)
-{
-    struct put_mac_binding *pmb;
-    HMAP_FOR_EACH_POP (pmb, hmap_node, &put_mac_bindings) {
-        uint32_t hash = hash_bytes(&pmb->ip_key, sizeof pmb->ip_key,
-                                   hash_2words(pmb->dp_key, pmb->port_key));
-        hmap_insert(&buffered_mac_bindings, &pmb->hmap_node, hash);
-    }
-}
-
 static void
 flush_put_mac_bindings(void)
 {
diff --git a/tests/ovn.at b/tests/ovn.at
index 34dba4edf..6499df3d3 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -2883,11 +2883,10 @@  for is in 1 2 3; do
               echo $arp >> $id$jd2$kd.expected
             done
           done
-          if test $(vif_to_hv ${is}${js}${ks}) = $(vif_to_hv ${id}${jd}1); then
-              hmac=8000000000$o4
-              rmac=00000000ff$id$jd
-              echo ${hmac}${rmac}08004500001c00000000"3f1101"00${sip}${dip}0035111100080000 >> ${id}11.expected
-          fi
+
+          hmac=8000000000$o4
+          rmac=00000000ff$id$jd
+          echo ${hmac}${rmac}08004500001c00000000"3f1101"00${sip}${dip}0035111100080000 >> ${id}11.expected
 
           host_mac=8000000000$o4
           lrmac=00000000ff$id$jd
@@ -3207,6 +3206,8 @@  sha=f00000000012
 test_arp 12 $sha $spa $tpa
 OVS_WAIT_UNTIL([ovn-sbctl find mac_binding ip="192.168.1.100" | grep f0:00:00:00:00:12])
 ovn-nbctl --wait=hv sync
+# give to the hv the time to send queued ip packets
+sleep 1
 
 # Send an IP packet from lp21 to 192.168.1.100, which should go to lp12.