mbox series

[ovs-dev,v2,0/3] add buffering support for IP packets

Message ID cover.1538487934.git.lorenzo.bianconi@redhat.com
Headers show
Series add buffering support for IP packets | expand

Message

Lorenzo Bianconi Oct. 2, 2018, 1:59 p.m. UTC
Add buffering support for IP traffic that will be processed
by neighboring subsystem (arp{} or nd_ns{} actions) since this
will result in the lost of the first packet of the connection

Changes since v1:
- add automatic test support for IPv4 and IPv6
- fix automatic tests broken by ip-buffering support
- reduce queue depth to 3 packets
- use binary form of the IP address as hash key
- in buffered_send_packets() routine signature use eth_addr instead of
  unsigned char pointer
- use unsigned int for {head/tail} queue index

Lorenzo Bianconi (3):
  OVN: add buffering support for ipv4 packets
  OVN: add buffering support for ipv6 packets
  OVN: fix automatic tests broken by ip-buffering support

 ovn/controller/pinctrl.c | 210 +++++++++++++++++++++++++++++++++++++--
 tests/ovn.at             | 153 +++++++++++++++++++++++++++-
 2 files changed, 355 insertions(+), 8 deletions(-)

Comments

Ben Pfaff Oct. 4, 2018, 6:33 p.m. UTC | #1
On Tue, Oct 02, 2018 at 03:59:11PM +0200, Lorenzo Bianconi wrote:
> Add buffering support for IP traffic that will be processed
> by neighboring subsystem (arp{} or nd_ns{} actions) since this
> will result in the lost of the first packet of the connection

Thanks for the revision.

The third patch seems like it should be folded into one of the others
(possibly the first?) because it's not good to knowingly add a patch
that breaks tests.  Instead, tests should be updated in the same patch
that would otherwise break them.

I'm not sure there's a reason to separate IPv4 and IPv6, either.  So I
would be inclined to squash these into a single patch.

I noticed a spelling error in the name of pinctrl_handle_bufferd_packets
("bufferd").

The customary type for ipv6 addresses is struct in6_addr.  I don't see
much benefit to using ovs_be128.  They are the same size but ovs_be128
doesn't have the same connotation.

I don't see much benefit to storing and hashing a string version of the
ipv6 address.  It's only needed in one place and we can generate it on
the fly there.

I'm appending an incremental that could be applied to the squashed
series.  Please let me know what you think of it.  I have tested that it
passes the unit tests.

Thanks a lot!

--8<--------------------------cut here-------------------------->8--

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 325f924477a3..69a811902fc0 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -206,7 +206,7 @@ struct buffered_packets {
     struct hmap_node hmap_node;
 
     /* key */
-    ovs_be128 ip;
+    struct in6_addr ip;
 
     long long int timestamp;
 
@@ -317,13 +317,13 @@ buffered_packets_map_gc(void)
 }
 
 static struct buffered_packets *
-pinctrl_find_buffered_packets(const ovs_be128 *ip, uint32_t hash)
+pinctrl_find_buffered_packets(const struct in6_addr *ip, uint32_t hash)
 {
     struct buffered_packets *qp;
 
     HMAP_FOR_EACH_WITH_HASH (qp, hmap_node, hash,
                              &buffered_packets_map) {
-        if (!memcmp(&qp->ip, ip, sizeof(ovs_be128))) {
+        if (IN6_ARE_ADDR_EQUAL(&qp->ip, ip)) {
             return qp;
         }
     }
@@ -331,9 +331,9 @@ pinctrl_find_buffered_packets(const ovs_be128 *ip, uint32_t hash)
 }
 
 static int
-pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
-                               struct dp_packet *pkt_in,
-                               const struct match *md, bool is_arp)
+pinctrl_handle_buffered_packets(const struct flow *ip_flow,
+                                struct dp_packet *pkt_in,
+                                const struct match *md, bool is_arp)
 {
     struct buffered_packets *bp;
     struct dp_packet *clone;
@@ -345,9 +345,8 @@ pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
         addr = ip_flow->ipv6_dst;
     }
 
-    ovs_be128 ip = get_32aligned_be128((const ovs_32aligned_be128 *)&addr);
-    uint32_t hash = hash_bytes(&ip, sizeof(ovs_be128), 0);
-    bp = pinctrl_find_buffered_packets(&ip, hash);
+    uint32_t hash = hash_bytes(&addr, sizeof addr, 0);
+    bp = pinctrl_find_buffered_packets(&addr, hash);
     if (!bp) {
         if (hmap_count(&buffered_packets_map) >= 1000) {
             COVERAGE_INC(pinctrl_drop_buffered_packets_map);
@@ -357,7 +356,7 @@ pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
         bp = xmalloc(sizeof *bp);
         hmap_insert(&buffered_packets_map, &bp->hmap_node, hash);
         bp->head = bp->tail = 0;
-        bp->ip = ip;
+        bp->ip = addr;
     }
     bp->timestamp = time_msec();
     /* clone the packet to send it later with correct L2 address */
@@ -381,7 +380,7 @@ pinctrl_handle_arp(const struct flow *ip_flow, struct dp_packet *pkt_in,
         return;
     }
 
-    pinctrl_handle_bufferd_packets(ip_flow, pkt_in, md, true);
+    pinctrl_handle_buffered_packets(ip_flow, pkt_in, md, true);
 
     /* Compose an ARP packet. */
     uint64_t packet_stub[128 / 8];
@@ -1815,7 +1814,7 @@ struct put_mac_binding {
     /* Key. */
     uint32_t dp_key;
     uint32_t port_key;
-    char ip_s[INET6_ADDRSTRLEN + 1];
+    struct in6_addr ip_key;
 
     /* Value. */
     struct eth_addr mac;
@@ -1839,13 +1838,13 @@ destroy_put_mac_bindings(void)
 
 static struct put_mac_binding *
 pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t port_key,
-                             const char *ip_s, uint32_t hash)
+                             const struct in6_addr *ip_key, uint32_t hash)
 {
     struct put_mac_binding *pa;
     HMAP_FOR_EACH_WITH_HASH (pa, hmap_node, hash, &put_mac_bindings) {
         if (pa->dp_key == dp_key
             && pa->port_key == port_key
-            && !strcmp(pa->ip_s, ip_s)) {
+            && IN6_ARE_ADDR_EQUAL(&pa->ip_key, ip_key)) {
             return pa;
         }
     }
@@ -1858,24 +1857,19 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
 {
     uint32_t dp_key = ntohll(md->metadata);
     uint32_t port_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
-    char ip_s[INET6_ADDRSTRLEN];
     struct buffered_packets *bp;
-    ovs_be128 ip_key;
+    struct in6_addr ip_key;
 
     if (is_arp) {
-        ovs_be32 ip = htonl(md->regs[0]);
-        inet_ntop(AF_INET, &ip, ip_s, sizeof(ip_s));
-
-        struct in6_addr addr = in6_addr_mapped_ipv4(ip);
-        ip_key = get_32aligned_be128((const ovs_32aligned_be128 *)&addr);
+        ip_key = in6_addr_mapped_ipv4(htonl(md->regs[0]));
     } else {
         ovs_be128 ip6 = hton128(flow_get_xxreg(md, 0));
-        inet_ntop(AF_INET6, &ip6, ip_s, sizeof(ip_s));
-        ip_key = ip6;
+        memcpy(&ip_key, &ip6, sizeof ip_key);
     }
-    uint32_t hash = hash_string(ip_s, hash_2words(dp_key, port_key));
+    uint32_t hash = hash_bytes(&ip_key, sizeof ip_key,
+                               hash_2words(dp_key, port_key));
     struct put_mac_binding *pmb
-        = pinctrl_find_put_mac_binding(dp_key, port_key, ip_s, hash);
+        = pinctrl_find_put_mac_binding(dp_key, port_key, &ip_key, hash);
     if (!pmb) {
         if (hmap_count(&put_mac_bindings) >= 1000) {
             COVERAGE_INC(pinctrl_drop_put_mac_binding);
@@ -1886,13 +1880,13 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
         hmap_insert(&put_mac_bindings, &pmb->hmap_node, hash);
         pmb->dp_key = dp_key;
         pmb->port_key = port_key;
-        ovs_strlcpy_arrays(pmb->ip_s, ip_s);
+        pmb->ip_key = ip_key;
     }
     pmb->timestamp = time_msec();
     pmb->mac = headers->dl_src;
 
     /* send queued pkts */
-    uint32_t bhash = hash_bytes(&ip_key, sizeof(ovs_be128), 0);
+    uint32_t bhash = hash_bytes(&ip_key, sizeof ip_key, 0);
     bp = pinctrl_find_buffered_packets(&ip_key, bhash);
     if (bp) {
         buffered_send_packets(bp, &pmb->mac);
@@ -1946,25 +1940,23 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
     snprintf(mac_string, sizeof mac_string,
              ETH_ADDR_FMT, ETH_ADDR_ARGS(pmb->mac));
 
-    /* Check for and update an existing IP-MAC binding for this logical
-     * port.
-     */
+    struct ds ip_s = DS_EMPTY_INITIALIZER;
+    ipv6_format_mapped(&pmb->ip_key, &ip_s);
+
+    /* Update or add an IP-MAC binding for this logical port. */
     const struct sbrec_mac_binding *b =
         mac_binding_lookup(sbrec_mac_binding_by_lport_ip, pb->logical_port,
-                           pmb->ip_s);
-    if (b) {
-        if (strcmp(b->mac, mac_string)) {
-            sbrec_mac_binding_set_mac(b, mac_string);
-        }
-        return;
-    }
-
-    /* Add new IP-MAC binding for this logical port. */
-    b = sbrec_mac_binding_insert(ovnsb_idl_txn);
-    sbrec_mac_binding_set_logical_port(b, pb->logical_port);
-    sbrec_mac_binding_set_ip(b, pmb->ip_s);
-    sbrec_mac_binding_set_mac(b, mac_string);
-    sbrec_mac_binding_set_datapath(b, pb->datapath);
+                           ds_cstr(&ip_s));
+    if (!b) {
+        b = sbrec_mac_binding_insert(ovnsb_idl_txn);
+        sbrec_mac_binding_set_logical_port(b, pb->logical_port);
+        sbrec_mac_binding_set_ip(b, ds_cstr(&ip_s));
+        sbrec_mac_binding_set_mac(b, mac_string);
+        sbrec_mac_binding_set_datapath(b, pb->datapath);
+    } else if (strcmp(b->mac, mac_string)) {
+        sbrec_mac_binding_set_mac(b, mac_string);
+    }
+    ds_destroy(&ip_s);
 }
 
 static void
@@ -2605,7 +2597,7 @@ pinctrl_handle_nd_ns(const struct flow *ip_flow, struct dp_packet *pkt_in,
         return;
     }
 
-    pinctrl_handle_bufferd_packets(ip_flow, pkt_in, md, false);
+    pinctrl_handle_buffered_packets(ip_flow, pkt_in, md, false);
 
     uint64_t packet_stub[128 / 8];
     struct dp_packet packet;
Lorenzo Bianconi Oct. 5, 2018, 4:41 p.m. UTC | #2
> On Tue, Oct 02, 2018 at 03:59:11PM +0200, Lorenzo Bianconi wrote:
> > Add buffering support for IP traffic that will be processed
> > by neighboring subsystem (arp{} or nd_ns{} actions) since this
> > will result in the lost of the first packet of the connection
> 
> Thanks for the revision.
> 
> The third patch seems like it should be folded into one of the others
> (possibly the first?) because it's not good to knowingly add a patch
> that breaks tests.  Instead, tests should be updated in the same patch
> that would otherwise break them.

ack, agree. Will do in v3

> 
> I'm not sure there's a reason to separate IPv4 and IPv6, either.  So I
> would be inclined to squash these into a single patch.
> 

ack. Will do in v3

> I noticed a spelling error in the name of pinctrl_handle_bufferd_packets
> ("bufferd").
> 
> The customary type for ipv6 addresses is struct in6_addr.  I don't see
> much benefit to using ovs_be128.  They are the same size but ovs_be128
> doesn't have the same connotation.

ack

> 
> I don't see much benefit to storing and hashing a string version of the
> ipv6 address.  It's only needed in one place and we can generate it on
> the fly there.
> 
> I'm appending an incremental that could be applied to the squashed
> series.  Please let me know what you think of it.  I have tested that it
> passes the unit tests.

Thanks, it works properly :)
I am sending v3 addressing your comments and adding your changes.

Regards,
Lorenzo

> 
> Thanks a lot!
> 
> --8<--------------------------cut here-------------------------->8--
> 
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 325f924477a3..69a811902fc0 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -206,7 +206,7 @@ struct buffered_packets {
>      struct hmap_node hmap_node;
>  
>      /* key */
> -    ovs_be128 ip;
> +    struct in6_addr ip;
>  
>      long long int timestamp;
>  
> @@ -317,13 +317,13 @@ buffered_packets_map_gc(void)
>  }
>  
>  static struct buffered_packets *
> -pinctrl_find_buffered_packets(const ovs_be128 *ip, uint32_t hash)
> +pinctrl_find_buffered_packets(const struct in6_addr *ip, uint32_t hash)
>  {
>      struct buffered_packets *qp;
>  
>      HMAP_FOR_EACH_WITH_HASH (qp, hmap_node, hash,
>                               &buffered_packets_map) {
> -        if (!memcmp(&qp->ip, ip, sizeof(ovs_be128))) {
> +        if (IN6_ARE_ADDR_EQUAL(&qp->ip, ip)) {
>              return qp;
>          }
>      }
> @@ -331,9 +331,9 @@ pinctrl_find_buffered_packets(const ovs_be128 *ip, uint32_t hash)
>  }
>  
>  static int
> -pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
> -                               struct dp_packet *pkt_in,
> -                               const struct match *md, bool is_arp)
> +pinctrl_handle_buffered_packets(const struct flow *ip_flow,
> +                                struct dp_packet *pkt_in,
> +                                const struct match *md, bool is_arp)
>  {
>      struct buffered_packets *bp;
>      struct dp_packet *clone;
> @@ -345,9 +345,8 @@ pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
>          addr = ip_flow->ipv6_dst;
>      }
>  
> -    ovs_be128 ip = get_32aligned_be128((const ovs_32aligned_be128 *)&addr);
> -    uint32_t hash = hash_bytes(&ip, sizeof(ovs_be128), 0);
> -    bp = pinctrl_find_buffered_packets(&ip, hash);
> +    uint32_t hash = hash_bytes(&addr, sizeof addr, 0);
> +    bp = pinctrl_find_buffered_packets(&addr, hash);
>      if (!bp) {
>          if (hmap_count(&buffered_packets_map) >= 1000) {
>              COVERAGE_INC(pinctrl_drop_buffered_packets_map);
> @@ -357,7 +356,7 @@ pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
>          bp = xmalloc(sizeof *bp);
>          hmap_insert(&buffered_packets_map, &bp->hmap_node, hash);
>          bp->head = bp->tail = 0;
> -        bp->ip = ip;
> +        bp->ip = addr;
>      }
>      bp->timestamp = time_msec();
>      /* clone the packet to send it later with correct L2 address */
> @@ -381,7 +380,7 @@ pinctrl_handle_arp(const struct flow *ip_flow, struct dp_packet *pkt_in,
>          return;
>      }
>  
> -    pinctrl_handle_bufferd_packets(ip_flow, pkt_in, md, true);
> +    pinctrl_handle_buffered_packets(ip_flow, pkt_in, md, true);
>  
>      /* Compose an ARP packet. */
>      uint64_t packet_stub[128 / 8];
> @@ -1815,7 +1814,7 @@ struct put_mac_binding {
>      /* Key. */
>      uint32_t dp_key;
>      uint32_t port_key;
> -    char ip_s[INET6_ADDRSTRLEN + 1];
> +    struct in6_addr ip_key;
>  
>      /* Value. */
>      struct eth_addr mac;
> @@ -1839,13 +1838,13 @@ destroy_put_mac_bindings(void)
>  
>  static struct put_mac_binding *
>  pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t port_key,
> -                             const char *ip_s, uint32_t hash)
> +                             const struct in6_addr *ip_key, uint32_t hash)
>  {
>      struct put_mac_binding *pa;
>      HMAP_FOR_EACH_WITH_HASH (pa, hmap_node, hash, &put_mac_bindings) {
>          if (pa->dp_key == dp_key
>              && pa->port_key == port_key
> -            && !strcmp(pa->ip_s, ip_s)) {
> +            && IN6_ARE_ADDR_EQUAL(&pa->ip_key, ip_key)) {
>              return pa;
>          }
>      }
> @@ -1858,24 +1857,19 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
>  {
>      uint32_t dp_key = ntohll(md->metadata);
>      uint32_t port_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
> -    char ip_s[INET6_ADDRSTRLEN];
>      struct buffered_packets *bp;
> -    ovs_be128 ip_key;
> +    struct in6_addr ip_key;
>  
>      if (is_arp) {
> -        ovs_be32 ip = htonl(md->regs[0]);
> -        inet_ntop(AF_INET, &ip, ip_s, sizeof(ip_s));
> -
> -        struct in6_addr addr = in6_addr_mapped_ipv4(ip);
> -        ip_key = get_32aligned_be128((const ovs_32aligned_be128 *)&addr);
> +        ip_key = in6_addr_mapped_ipv4(htonl(md->regs[0]));
>      } else {
>          ovs_be128 ip6 = hton128(flow_get_xxreg(md, 0));
> -        inet_ntop(AF_INET6, &ip6, ip_s, sizeof(ip_s));
> -        ip_key = ip6;
> +        memcpy(&ip_key, &ip6, sizeof ip_key);
>      }
> -    uint32_t hash = hash_string(ip_s, hash_2words(dp_key, port_key));
> +    uint32_t hash = hash_bytes(&ip_key, sizeof ip_key,
> +                               hash_2words(dp_key, port_key));
>      struct put_mac_binding *pmb
> -        = pinctrl_find_put_mac_binding(dp_key, port_key, ip_s, hash);
> +        = pinctrl_find_put_mac_binding(dp_key, port_key, &ip_key, hash);
>      if (!pmb) {
>          if (hmap_count(&put_mac_bindings) >= 1000) {
>              COVERAGE_INC(pinctrl_drop_put_mac_binding);
> @@ -1886,13 +1880,13 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
>          hmap_insert(&put_mac_bindings, &pmb->hmap_node, hash);
>          pmb->dp_key = dp_key;
>          pmb->port_key = port_key;
> -        ovs_strlcpy_arrays(pmb->ip_s, ip_s);
> +        pmb->ip_key = ip_key;
>      }
>      pmb->timestamp = time_msec();
>      pmb->mac = headers->dl_src;
>  
>      /* send queued pkts */
> -    uint32_t bhash = hash_bytes(&ip_key, sizeof(ovs_be128), 0);
> +    uint32_t bhash = hash_bytes(&ip_key, sizeof ip_key, 0);
>      bp = pinctrl_find_buffered_packets(&ip_key, bhash);
>      if (bp) {
>          buffered_send_packets(bp, &pmb->mac);
> @@ -1946,25 +1940,23 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
>      snprintf(mac_string, sizeof mac_string,
>               ETH_ADDR_FMT, ETH_ADDR_ARGS(pmb->mac));
>  
> -    /* Check for and update an existing IP-MAC binding for this logical
> -     * port.
> -     */
> +    struct ds ip_s = DS_EMPTY_INITIALIZER;
> +    ipv6_format_mapped(&pmb->ip_key, &ip_s);
> +
> +    /* Update or add an IP-MAC binding for this logical port. */
>      const struct sbrec_mac_binding *b =
>          mac_binding_lookup(sbrec_mac_binding_by_lport_ip, pb->logical_port,
> -                           pmb->ip_s);
> -    if (b) {
> -        if (strcmp(b->mac, mac_string)) {
> -            sbrec_mac_binding_set_mac(b, mac_string);
> -        }
> -        return;
> -    }
> -
> -    /* Add new IP-MAC binding for this logical port. */
> -    b = sbrec_mac_binding_insert(ovnsb_idl_txn);
> -    sbrec_mac_binding_set_logical_port(b, pb->logical_port);
> -    sbrec_mac_binding_set_ip(b, pmb->ip_s);
> -    sbrec_mac_binding_set_mac(b, mac_string);
> -    sbrec_mac_binding_set_datapath(b, pb->datapath);
> +                           ds_cstr(&ip_s));
> +    if (!b) {
> +        b = sbrec_mac_binding_insert(ovnsb_idl_txn);
> +        sbrec_mac_binding_set_logical_port(b, pb->logical_port);
> +        sbrec_mac_binding_set_ip(b, ds_cstr(&ip_s));
> +        sbrec_mac_binding_set_mac(b, mac_string);
> +        sbrec_mac_binding_set_datapath(b, pb->datapath);
> +    } else if (strcmp(b->mac, mac_string)) {
> +        sbrec_mac_binding_set_mac(b, mac_string);
> +    }
> +    ds_destroy(&ip_s);
>  }
>  
>  static void
> @@ -2605,7 +2597,7 @@ pinctrl_handle_nd_ns(const struct flow *ip_flow, struct dp_packet *pkt_in,
>          return;
>      }
>  
> -    pinctrl_handle_bufferd_packets(ip_flow, pkt_in, md, false);
> +    pinctrl_handle_buffered_packets(ip_flow, pkt_in, md, false);
>  
>      uint64_t packet_stub[128 / 8];
>      struct dp_packet packet;