On Thu, Jul 17, 2014 at 8:20 PM, Wenyu Zhang <wen...@vmware.com> wrote: > > > -----Original Message----- > From: Pravin Shelar [mailto:pshe...@nicira.com] > Sent: Friday, July 18, 2014 1:20 AM > To: Wenyu Zhang > Cc: Romain Lenglet; Ben Pfaff; Jesse Gross; dev@openvswitch.org > Subject: Re: [PATCH v3] Extend OVS IPFIX exporter to export tunnel headers > > On Thu, Jul 17, 2014 at 3:05 AM, Wenyu Zhang <wen...@vmware.com> wrote: >> Thanks for the review. >> >> I have removed the incorrect whitespaces. >> >> About XXX_get_out_tun_info(), I keep the caluclation of saddr, including >> route look up process, tp_src and tp_dst in ops for each vport, due to that >> the process are a little different for each vport. And I think that the >> potential changes for vport ops in future may affect the process too, it's >> better to keep this process in vport ops. >> And the operation to fill the collected info into out_tun_info data is >> exactly same, I abstract a function ovs_flow_get_out_tun_info to do it. >> > All these function do routing lookup and then copy out key, so you can pass > specific parameter to common function to generate out_key. > > Wenyu: I have moved the process to copy out key into a common fucntion named > ovs_flow_get_out_tun_info(). > About the routing lookup, which is used to get the source IP > address, and same as the process in XXX_send(). I am not sure whether the > process to get source IP address may be different for different vports in > future. So I think that it's better to keep it in the vport ops. And the > algrithom to get TCP/UDP source/dest port is entirely different for each > vport, I keep it in the vport ops too.
I agree the process can change in future and we can change code accordingly later. For now lets keep avoid duplicate code. > And then the calculated source IP address, TCP/UDP source/dest > port are passed to the common function to generate the out_tun_info. ok. > > >> I have sent the freshed patch just a moment ago. May you help? >> >> Bests, >> Wenyu >> >> -----Original Message----- >> From: Pravin Shelar [mailto:pshe...@nicira.com] >> Sent: Thursday, July 17, 2014 1:03 AM >> To: Wenyu Zhang >> Cc: Romain Lenglet; Ben Pfaff; Jesse Gross; dev@openvswitch.org >> Subject: Re: [PATCH v3] Extend OVS IPFIX exporter to export tunnel >> headers >> >> git am warning: >> Applying: Extend OVS IPFIX exporter to export tunnel headers >> /home/pravin/ovs/w8/.git/rebase-apply/patch:143: trailing whitespace. >> if (out_tun_key) >> /home/pravin/ovs/w8/.git/rebase-apply/patch:236: space before tab in indent. >> const struct udphdr * udp_hdr = tph; >> /home/pravin/ovs/w8/.git/rebase-apply/patch:433: trailing whitespace. >> /home/pravin/ovs/w8/.git/rebase-apply/patch:594: trailing whitespace. >> /home/pravin/ovs/w8/.git/rebase-apply/patch:695: trailing whitespace. >> warning: squelched 21 whitespace errors >> warning: 26 lines add whitespace errors. >> >> >> >> On Wed, Jul 16, 2014 at 12:55 AM, Wenyu Zhang <wen...@vmware.com> wrote: >>> Extend IPFIX exporter to export tunnel headers when both input and >>> output of the port. >>> Add three other_config options in IPFIX table: enable-input-sampling, >>> enable-output-sampling and enable-tunnel-sampling, to control whether >>> sampling tunnel info, on which direction (input or output). >>> Insert sampling action before output action and the output tunnel >>> port is sent to datapath in the sampling action. >>> Make datapath collect output tunnel info and send it back to userpace >>> in upcall message with a new additional optional attribute. >>> Add a tunnel ports map to make the tunnel port lookup faster in >>> sampling upcalls in IPFIX exporter. Make the IPFIX exporter generate >>> IPFIX template sets with enterprise elements for the tunnel info, >>> save the tunnel info in IPFIX cache entries, and send IPFIX DATA with >>> tunnel info. >>> Add flowDirection element in IPFIX templates. >>> >>> Signed-off-by: Wenyu Zhang <wen...@vmware.com> >>> Acked-by: Romain Lenglet <rleng...@vmware.com> >>> --- >>> datapath/actions.c | 16 +- >>> datapath/datapath.c | 42 +++- >>> datapath/datapath.h | 2 + >>> datapath/flow.h | 24 +- >>> datapath/flow_netlink.c | 58 ++++- >>> datapath/flow_netlink.h | 2 + >>> datapath/vport-geneve.c | 63 ++++- >>> datapath/vport-gre.c | 67 ++++- >>> datapath/vport-lisp.c | 67 ++++- >>> datapath/vport-vxlan.c | 71 +++++- >>> datapath/vport.h | 4 + >>> include/linux/openvswitch.h | 21 +- >>> lib/dpif-linux.c | 2 + >>> lib/dpif.h | 1 + >>> lib/odp-util.c | 183 +++++++++----- >>> lib/odp-util.h | 2 + >>> lib/packets.h | 9 +- >>> ofproto/automake.mk | 4 + >>> ofproto/ipfix-enterprise-entities.def | 19 ++ >>> ofproto/ofproto-dpif-ipfix.c | 444 >>> ++++++++++++++++++++++++++++++--- >>> ofproto/ofproto-dpif-ipfix.h | 14 +- >>> ofproto/ofproto-dpif-upcall.c | 20 +- >>> ofproto/ofproto-dpif-xlate.c | 51 +++- >>> ofproto/ofproto-dpif.c | 20 ++ >>> ofproto/ofproto.h | 3 + >>> ofproto/tunnel.c | 4 + >>> tests/odp.at | 9 +- >>> utilities/ovs-vsctl.8.in | 8 +- >>> vswitchd/bridge.c | 9 + >>> vswitchd/vswitch.ovsschema | 5 +- >>> vswitchd/vswitch.xml | 27 ++ >>> 31 files changed, 1097 insertions(+), 174 deletions(-) create mode >>> 100644 ofproto/ipfix-enterprise-entities.def >>> >>> diff --git a/datapath/actions.c b/datapath/actions.c index >>> 39a21f4..226b849 100644 >>> --- a/datapath/actions.c >>> +++ b/datapath/actions.c >> ... >> >>> static inline void ovs_flow_tun_info_init(struct ovs_tunnel_info *tun_info, >>> - const struct iphdr *iph, __be64 >>> tun_id, >>> + const struct iphdr *iph, const >>> void *tph, >>> + __be64 tun_id, >>> __be16 tun_flags, >>> struct geneve_opt *opts, >>> u8 opts_len) @@ -79,6 +82,21 >>> @@ static inline void ovs_flow_tun_info_init(struct ovs_tunnel_info >>> *tun_info, >>> tun_info->tunnel.ipv4_ttl = iph->ttl; >>> tun_info->tunnel.tun_flags = tun_flags; >>> >>> + /* For the tunnel types on the top of IPsec,the tp_src and tp_dst of >>> + * the upper tunnel are used. >>> + * E.g: GRE over IPSEC, the tp_src and tp_port are zero. >>> + */ >>> + if (tph && >>> + (iph->protocol == IPPROTO_UDP || >>> + iph->protocol == IPPROTO_TCP)) { >>> + const struct udphdr * udp_hdr = tph; >>> + tun_info->tunnel.tp_src = udp_hdr->source; >>> + tun_info->tunnel.tp_dst = udp_hdr->dest; >>> + } else { >>> + tun_info->tunnel.tp_src = 0; >>> + tun_info->tunnel.tp_dst = 0; >>> + } >>> + >> >> Can you pass source port and dst post to to this function, so that we can >> avoid these checks this function. >> >>> /* clear struct padding. */ >>> memset((unsigned char *) &tun_info->tunnel + OVS_TUNNEL_KEY_SIZE, 0, >>> sizeof(tun_info->tunnel) - OVS_TUNNEL_KEY_SIZE); diff >>> --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c index >>> 5f975a1..1550daa 100644 >>> --- a/datapath/flow_netlink.c >> .... >> >>> >>> +static int geneve_get_out_tun_info(struct vport *vport, struct sk_buff >>> *skb, >>> + struct ovs_tunnel_info >>> +*out_tun_info) { >>> + struct geneve_port *geneve_port = geneve_vport(vport); >>> + struct ovs_key_ipv4_tunnel *tun_key; >>> + struct ovs_key_ipv4_tunnel *out_tun_key = &out_tun_info->tunnel; >>> + struct rtable *rt; >>> + __be32 saddr; >>> + int err = 0; >>> + >>> + >>> + if (unlikely(!OVS_CB(skb)->tun_info)) >>> + return -EINVAL; >>> + >>> + tun_key = &OVS_CB(skb)->tun_info->tunnel; >>> + >>> + /* Route lookup */ >>> + saddr = tun_key->ipv4_src; >>> + rt = find_route(ovs_dp_get_net(vport->dp), >>> + &saddr, tun_key->ipv4_dst, >>> + IPPROTO_UDP, tun_key->ipv4_tos, >>> + skb->mark); >>> + if (IS_ERR(rt)) { >>> + err = PTR_ERR(rt); >>> + goto error; >>> + } >>> + ip_rt_put(rt); >>> + >>> + memset(out_tun_info, 0, sizeof(*out_tun_key)); >>> + /* Now the tun_key should contain the output tun key*/ >>> + out_tun_key->tun_flags |= TUNNEL_KEY; >>> + out_tun_key->tun_id = tun_key->tun_id; >>> + out_tun_key->ipv4_src = saddr; >>> + out_tun_key->ipv4_dst = tun_key->ipv4_dst; >>> + out_tun_key->ipv4_tos = tun_key->ipv4_tos; >>> + out_tun_key->ipv4_ttl = tun_key->ipv4_ttl; >>> + /* Get tp_src and tp_dst, refert to geneve_build_header() */ >>> + out_tun_key->tp_src = vxlan_src_port(1, USHRT_MAX, skb); >>> + out_tun_key->tp_dst = inet_sport(geneve_port->sock->sk); >>> + /* Set GENEVE's option */ >>> + out_tun_info->options = OVS_CB(skb)->tun_info->options; >>> + out_tun_info->options_len = >>> + OVS_CB(skb)->tun_info->options_len; >>> + >>> +error: >>> + return err; >>> +} >>> + >>> const struct vport_ops ovs_geneve_vport_ops = { >>> - .type = OVS_VPORT_TYPE_GENEVE, >>> - .create = geneve_tnl_create, >>> - .destroy = geneve_tnl_destroy, >>> - .get_name = geneve_get_name, >>> - .get_options = geneve_get_options, >>> - .send = geneve_send, >>> + .type = OVS_VPORT_TYPE_GENEVE, >>> + .create = geneve_tnl_create, >>> + .destroy = geneve_tnl_destroy, >>> + .get_name = geneve_get_name, >>> + .get_options = geneve_get_options, >>> + .send = geneve_send, >>> + .get_out_tun_info = geneve_get_out_tun_info, >>> }; >> >> geneve_get_out_tun_info(), gre_get_out_tun_info(), vxlan_get_out_tun_info(), >> lisp_get_out_tun_info() has basically same code. Can you write one function >> then call that from all of these functions? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
