On Thu, Jun 19, 2014 at 1:30 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Tue, Jun 10, 2014 at 4:47 PM, Jesse Gross <je...@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 <pshe...@nicira.com>
>> Signed-off-by: Jesse Gross <je...@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.
I'm not sure I understand what you mean here - each protocol has a
different check for the header being present. What would a combined
version look like?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
