Thanks for the review, my comments inline. Bests, Wenyu -----Original Message----- From: Pravin Shelar [mailto:pshe...@nicira.com] Sent: Tuesday, July 22, 2014 6:56 AM To: Wenyu Zhang Cc: Romain Lenglet; Ben Pfaff; Jesse Gross; dev@openvswitch.org Subject: Re: [PATCH v5] Extend OVS IPFIX exporter to export tunnel headers
On Mon, Jul 21, 2014 at 2:39 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> This looking close, I have few comments below. > --- > v2: Address Romain's comments > v3: Address Pravin's comments, make datapath sent all tunnel info, > not only tunnel key to userspace. > v4: Address Pravin's comments, introduce a common function to get output > tunnel info for all vports, remove duplicated codes. > Rebase. > v5: Address Pravin's comments on v4, correct sparse errors, make a common > function to setup tunnel info data for both input and output case. > --- > datapath/actions.c | 18 ++ > datapath/datapath.c | 46 +++- > datapath/datapath.h | 2 + > datapath/flow.h | 54 ++-- > datapath/flow_netlink.c | 58 ++++- > datapath/flow_netlink.h | 2 + > datapath/vport-geneve.c | 36 ++- > datapath/vport-gre.c | 35 ++- > datapath/vport-lisp.c | 40 ++- > datapath/vport-vxlan.c | 39 ++- > datapath/vport.c | 40 +++ > datapath/vport.h | 11 + > 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 | 3 + > 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 ++ > 32 files changed, 1048 insertions(+), 187 deletions(-) > create mode 100644 ofproto/ipfix-enterprise-entities.def > > diff --git a/datapath/actions.c b/datapath/actions.c > index 39a21f4..c1ad4dc 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c > @@ -502,6 +502,7 @@ static int output_userspace(struct datapath *dp, struct > sk_buff *skb, > struct dp_upcall_info upcall; > const struct nlattr *a; > int rem; > + struct ovs_tunnel_info output_tunnel_info; > > BUG_ON(!OVS_CB(skb)->pkt_key); > > @@ -509,6 +510,7 @@ static int output_userspace(struct datapath *dp, struct > sk_buff *skb, > upcall.key = OVS_CB(skb)->pkt_key; > upcall.userdata = NULL; > upcall.portid = 0; > + upcall.out_tun_info = NULL; > > for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > a = nla_next(a, &rem)) { > @@ -520,7 +522,23 @@ static int output_userspace(struct datapath *dp, struct > sk_buff *skb, > case OVS_USERSPACE_ATTR_PID: > upcall.portid = nla_get_u32(a); > break; > + > + case OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT: { > + /* Get out tunnel info. */ > + int err; > + struct vport *vport; > + vport = ovs_vport_ovsl_rcu(dp, nla_get_u32(a)); This function is never called under ovs_lock, so you can use rcu version of lookup. Wenyu: Sorry, I am confused. I have used ovs_vport_ovsl_rcu(), do you mean another rcu version of lookup? > + if (vport->ops->get_out_tun_info){ You also need to check for vport, if it is NULL. Wenyu: Sure, I will add check for vport and vport->ops. > + err = vport->ops->get_out_tun_info( > + vport, skb, &output_tunnel_info); > + if (err == 0) { > + upcall.out_tun_info = > &output_tunnel_info; > + } > + } > + break; > } > + > + }/* End of switch. */ > } > ....... > { > + BUILD_BUG_ON(sizeof(tun_info->tunnel) != OVS_TUNNEL_KEY_SIZE); > + > tun_info->tunnel.tun_id = tun_id; > - tun_info->tunnel.ipv4_src = iph->saddr; > - tun_info->tunnel.ipv4_dst = iph->daddr; > - tun_info->tunnel.ipv4_tos = iph->tos; > - tun_info->tunnel.ipv4_ttl = iph->ttl; > + tun_info->tunnel.ipv4_src = saddr; > + tun_info->tunnel.ipv4_dst = daddr; > + tun_info->tunnel.ipv4_tos = tos; > + tun_info->tunnel.ipv4_ttl = ttl; > tun_info->tunnel.tun_flags = tun_flags; > > - /* clear struct padding. */ > - memset((unsigned char *) &tun_info->tunnel + OVS_TUNNEL_KEY_SIZE, 0, > - sizeof(tun_info->tunnel) - OVS_TUNNEL_KEY_SIZE); We still need this memset for case where we add another member to this struct, you can check for size of padding to avoid sparse warning. Wenyu: The additional if condition may affect performance, we'd better to add some comments to mention this usage. I have added a compling check about the size: BUILD_BUG_ON(sizeof(tun_info->tunnel) != OVS_TUNNEL_KEY_SIZE); If we add another member to this structre in future, this check will failed when compling to mention us, and then we can add the memset only when the failure happens. And I can add some comments about the usage of memset here. > + /* 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. > + */ > + tun_info->tunnel.tp_src = tp_src; > + tun_info->tunnel.tp_dst = tp_dst; > > tun_info->options = opts; > tun_info->options_len = opts_len; > } > ..... > > +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); > + > + if (unlikely(!OVS_CB(skb)->tun_info)) > + return -EINVAL; > + Can you move this check inside ovs_vport_out_tun_info. Wenyu: Considering that the tp_src and tp_dst need be prepared before ovs_vport_get_out_tun_info() is called, we'd better do the check before doing the work to calculate tp_src and tp_dst. So I perfer to keep the check here. > + /* > + * Get tp_src and tp_dst, refert to geneve_build_header(). > + */ > + return ovs_vport_get_out_tun_info(out_tun_info, OVS_CB(skb)->tun_info, > + ovs_dp_get_net(vport->dp), > + IPPROTO_UDP, skb->mark, > + vxlan_src_port(1, USHRT_MAX, skb), > + inet_sport(geneve_port->sock->sk)); > + > +} > + > 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, > }; _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev