[ovs-dev] [PATCH 2/7] datapath: Eliminate memset() from flow_extract.

Pravin Shelar pshelar at nicira.com
Thu Jun 19 20:30:34 UTC 2014


On Tue, Jun 10, 2014 at 4:47 PM, Jesse Gross <jesse at nicira.com> wrote:
> As new protocols are added, the size of the flow key tends to
> increase although few protocols care about all of the fields. In
> order to optimize this for hashing and matching, OVS uses a varible
> length portion of the key. However, when fields are extracted from
> the packet we must still zero out the entire key.
>
> This is no longer necessary now that OVS implements masking. Any
> fields (or holes in the structure) which are not part of a given
> protocol will be by definition not part of the mask and zeroed out
> during lookup. Furthermore, since masking already uses variable
> length keys this zeroing operation automatically benefits as well.
>
> In principle, the only thing that needs to be done at this point
> is remove the memset() at the beginning of flow. However, some
> fields assume that they are initialized to zero, which now must be
> done explicitly. In addition, in the event of an error we must also
> zero out corresponding fields to signal that there is no valid data
> present. These increase the total amount of code but very little of
> it is executed in non-error situations.
>
> Removing the memset() reduces the profile of ovs_flow_extract()
> from 0.64% to 0.56% when tested with large packets on a 10G link.
>
> Suggested-by: Pravin Shelar <pshelar at nicira.com>
> Signed-off-by: Jesse Gross <jesse at nicira.com>
> ---
>  datapath/flow.c | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index c52081b..2a839ff 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -273,6 +273,8 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
>                         key->ip.frag = OVS_FRAG_TYPE_LATER;
>                 else
>                         key->ip.frag = OVS_FRAG_TYPE_FIRST;
> +       } else {
> +               key->ip.frag = OVS_FRAG_TYPE_NONE;
>         }
>
>         nh_len = payload_ofs - nh_ofs;
> @@ -357,6 +359,7 @@ static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
>          */
>         key->tp.src = htons(icmp->icmp6_type);
>         key->tp.dst = htons(icmp->icmp6_code);
> +       memset(&key->ipv6.nd, 0, sizeof(key->ipv6.nd));
>
>         if (icmp->icmp6_code == 0 &&
>             (icmp->icmp6_type == NDISC_NEIGHBOUR_SOLICITATION ||
> @@ -448,13 +451,19 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>         int error;
>         struct ethhdr *eth;
>
> -       memset(key, 0, sizeof(*key));
> -
>         key->phy.priority = skb->priority;
>         if (OVS_CB(skb)->tun_key)
>                 memcpy(&key->tun_key, OVS_CB(skb)->tun_key, sizeof(key->tun_key));
> +       else
> +               memset(&key->tun_key, 0, sizeof(key->tun_key));
> +
>         key->phy.in_port = in_port;
>         key->phy.skb_mark = skb->mark;
> +       key->ovs_flow_hash = 0;
> +       key->recirc_id = 0;
> +
> +       /* Flags are always used as part of stats. */
> +       key->tp.flags = 0;
>
>         skb_reset_mac_header(skb);
>
> @@ -469,6 +478,7 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>         /* We are going to push all headers that we pull, so no need to
>          * update skb->csum here. */
>
> +       key->eth.tci = 0;
>         if (vlan_tx_tag_present(skb))
>                 key->eth.tci = htons(vlan_get_tci(skb));
>         else if (eth->h_proto == htons(ETH_P_8021Q))
> @@ -489,6 +499,8 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>
>                 error = check_iphdr(skb);
>                 if (unlikely(error)) {
> +                       memset(&key->ip, 0, sizeof(key->ip));
> +                       memset(&key->ipv4, 0, sizeof(key->ipv4));
>                         if (error == -EINVAL) {
>                                 skb->transport_header = skb->network_header;
>                                 error = 0;
> @@ -512,6 +524,8 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>                 if (nh->frag_off & htons(IP_MF) ||
>                          skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
>                         key->ip.frag = OVS_FRAG_TYPE_FIRST;
> +               else
> +                       key->ip.frag = OVS_FRAG_TYPE_NONE;
>
>                 /* Transport layer. */
>                 if (key->ip.proto == IPPROTO_TCP) {
> @@ -520,18 +534,24 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>                                 key->tp.src = tcp->source;
>                                 key->tp.dst = tcp->dest;
>                                 key->tp.flags = TCP_FLAGS_BE16(tcp);
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
>                 } else if (key->ip.proto == IPPROTO_UDP) {
>                         if (udphdr_ok(skb)) {
>                                 struct udphdr *udp = udp_hdr(skb);
>                                 key->tp.src = udp->source;
>                                 key->tp.dst = udp->dest;
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
if we combine above if condition we can have common memset for key->tp.
>                 } else if (key->ip.proto == IPPROTO_SCTP) {
>                         if (sctphdr_ok(skb)) {
>                                 struct sctphdr *sctp = sctp_hdr(skb);
>                                 key->tp.src = sctp->source;
>                                 key->tp.dst = sctp->dest;
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
>                 } else if (key->ip.proto == IPPROTO_ICMP) {
>                         if (icmphdr_ok(skb)) {
> @@ -541,16 +561,19 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>                                  * them in 16-bit network byte order. */
>                                 key->tp.src = htons(icmp->type);
>                                 key->tp.dst = htons(icmp->code);
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
>                 }
>
> -       } else if ((key->eth.type == htons(ETH_P_ARP) ||
> -                  key->eth.type == htons(ETH_P_RARP)) && arphdr_ok(skb)) {
> +       } else if (key->eth.type == htons(ETH_P_ARP) ||
> +                  key->eth.type == htons(ETH_P_RARP)) {
>                 struct arp_eth_header *arp;
>
>                 arp = (struct arp_eth_header *)skb_network_header(skb);
>
> -               if (arp->ar_hrd == htons(ARPHRD_ETHER)
> +               if (arphdr_ok(skb)
> +                               && arp->ar_hrd == htons(ARPHRD_ETHER)
>                                 && arp->ar_pro == htons(ETH_P_IP)
>                                 && arp->ar_hln == ETH_ALEN
>                                 && arp->ar_pln == 4) {
> @@ -558,16 +581,24 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>                         /* We only match on the lower 8 bits of the opcode. */
>                         if (ntohs(arp->ar_op) <= 0xff)
>                                 key->ip.proto = ntohs(arp->ar_op);
> +                       else
> +                               key->ip.proto = 0;
> +
>                         memcpy(&key->ipv4.addr.src, arp->ar_sip, sizeof(key->ipv4.addr.src));
>                         memcpy(&key->ipv4.addr.dst, arp->ar_tip, sizeof(key->ipv4.addr.dst));
>                         ether_addr_copy(key->ipv4.arp.sha, arp->ar_sha);
>                         ether_addr_copy(key->ipv4.arp.tha, arp->ar_tha);
> +               } else {
> +                       memset(&key->ip, 0, sizeof(key->ip));
> +                       memset(&key->ipv4, 0, sizeof(key->ipv4));
>                 }
>         } else if (key->eth.type == htons(ETH_P_IPV6)) {
>                 int nh_len;             /* IPv6 Header + Extensions */
>
>                 nh_len = parse_ipv6hdr(skb, key);
>                 if (unlikely(nh_len < 0)) {
> +                       memset(&key->ip, 0, sizeof(key->ip));
> +                       memset(&key->ipv6.addr, 0, sizeof(key->ipv6.addr));
>                         if (nh_len == -EINVAL) {
>                                 skb->transport_header = skb->network_header;
>                                 error = 0;
> @@ -589,24 +620,32 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>                                 key->tp.src = tcp->source;
>                                 key->tp.dst = tcp->dest;
>                                 key->tp.flags = TCP_FLAGS_BE16(tcp);
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
>                 } else if (key->ip.proto == NEXTHDR_UDP) {
>                         if (udphdr_ok(skb)) {
>                                 struct udphdr *udp = udp_hdr(skb);
>                                 key->tp.src = udp->source;
>                                 key->tp.dst = udp->dest;
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
>                 } else if (key->ip.proto == NEXTHDR_SCTP) {
>                         if (sctphdr_ok(skb)) {
>                                 struct sctphdr *sctp = sctp_hdr(skb);
>                                 key->tp.src = sctp->source;
>                                 key->tp.dst = sctp->dest;
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
>                 } else if (key->ip.proto == NEXTHDR_ICMP) {
>                         if (icmp6hdr_ok(skb)) {
>                                 error = parse_icmpv6(skb, key, nh_len);
>                                 if (error)
>                                         return error;
> +                       } else {
> +                               memset(&key->tp, 0, sizeof(key->tp));
>                         }
same as above, if we combine the condition we can have single memset for tp.

>                 }
>         }

Otherwise Looks good.

Acked-by: Pravin B Shelar <pshelar at nicira.com>

> --
> 1.9.1
>



More information about the dev mailing list