[ovs-dev] [PATCH v2 0/3] add buffering support for IP packets
Lorenzo Bianconi
lorenzo.bianconi at redhat.com
Fri Oct 5 16:41:17 UTC 2018
> 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;
More information about the dev
mailing list