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.
> 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
