On Thu, Jun 19, 2014 at 1:46 PM, Jesse Gross <je...@nicira.com> wrote:
> 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?
I was thinking of something like:
if (key->ip.proto == IPPROTO_TCP && tcphdr_ok(skb)) {
struct tcphdr *tcp = tcp_hdr(skb);
key->tp.src = tcp->source;
key->tp.dst = tcp->dest;
key->tp.flags = TCP_FLAGS_BE16(tcp);
} else if (key->ip.proto == IPPROTO_UDP &&udphdr_ok(skb)) {
.....
.....
}else if (...) {
.....
} else {
memset(&key->tp, 0, sizeof(key->tp));
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
