sparse warning
CHECK /home/pravin/ovs/w8/datapath/linux/vport.c
/home/pravin/ovs/w8/datapath/linux/../flow.h:126:15: warning: memset
with byte count of 0
CC [M] /home/pravin/ovs/w8/datapath/linux/vport.o
On Fri, Jul 18, 2014 at 1:59 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 | 55 +++-
> datapath/flow_netlink.c | 58 ++++-
> datapath/flow_netlink.h | 2 +
> datapath/vport-geneve.c | 37 ++-
> datapath/vport-gre.c | 35 ++-
> datapath/vport-lisp.c | 39 ++-
> datapath/vport-vxlan.c | 38 ++-
> datapath/vport.c | 33 +++
> 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, 1045 insertions(+), 177 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
> @@ -501,7 +501,9 @@ static int output_userspace(struct datapath *dp, struct
> sk_buff *skb,
> {
> struct dp_upcall_info upcall;
> const struct nlattr *a;
> - int rem;
> + int rem, err;
> + struct ovs_tunnel_info output_tunnel_info;
> + struct vport *vport;
>
> BUG_ON(!OVS_CB(skb)->pkt_key);
>
> @@ -509,6 +511,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,6 +523,17 @@ 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*/
> + vport = ovs_vport_ovsl_rcu(dp, nla_get_u32(a));
> + if (vport->ops->get_out_tun_info){
> + err = vport->ops->get_out_tun_info(vport,
> skb, &output_tunnel_info);
> + if (err == 0) {
> + upcall.out_tun_info =
> &output_tunnel_info;
> + }
> + }
> + break;
> }
> }
>
According to ovs coding style, variable are declared in local block,
so you can move vport and err to respective local block.
In case of error from get_out_tun_info() or invalid vport, we can skip
user upcall, since userspace will discard this data anyways.
....
>
> static size_t upcall_msg_size(const struct nlattr *userdata,
> + bool out_tun_key,
> unsigned int hdrlen)
> {
> size_t size = NLMSG_ALIGN(sizeof(struct ovs_header))
> @@ -418,6 +429,10 @@ static size_t upcall_msg_size(const struct nlattr
> *userdata,
> if (userdata)
> size += NLA_ALIGN(userdata->nla_len);
>
> + /* OVS_PACKET_ATTR_OUT_TUNNEL_KEY */
> + if (out_tun_key)
> + size += nla_total_size(tun_key_attr_size());
> +
> return size;
> }
>
Can you pass down upcall_info to upcall_msg_size(), it look more
consistent that way.
> static inline void ovs_flow_tun_info_init(struct ovs_tunnel_info *tun_info,
> - const struct iphdr *iph, __be64
> tun_id,
> - __be16 tun_flags,
> - struct geneve_opt *opts,
> - u8 opts_len)
> + const struct iphdr *iph,
> + const void *tph,
> + __be64 tun_id,
> + __be16 tun_flags,
> + struct geneve_opt *opts,
> + u8 opts_len)
> {
Rather than passing down tph, can just pass tp_src_port and
tp_dst_port? we can avoid if conditions in this function.
> tun_info->tunnel.tun_id = tun_id;
> tun_info->tunnel.ipv4_src = iph->saddr;
> @@ -79,6 +83,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;
> + }
> +
> /* clear struct padding. */
> memset((unsigned char *) &tun_info->tunnel + OVS_TUNNEL_KEY_SIZE, 0,
> sizeof(tun_info->tunnel) - OVS_TUNNEL_KEY_SIZE);
> @@ -87,6 +106,30 @@ static inline void ovs_flow_tun_info_init(struct
> ovs_tunnel_info *tun_info,
> tun_info->options_len = opts_len;
> }
>
> +static inline void ovs_flow_out_tun_info_init(struct ovs_tunnel_info
> *out_tun_info,
> + const struct ovs_tunnel_info
> *tun_info,
> + __be32 saddr,
> + __be16 tp_src,
> + __be16 tp_dst)
> +{
> + out_tun_info->tunnel.tun_id = tun_info->tunnel.tun_id;
> + out_tun_info->tunnel.ipv4_src = saddr;
> + out_tun_info->tunnel.ipv4_dst = tun_info->tunnel.ipv4_dst;
> + out_tun_info->tunnel.ipv4_tos = tun_info->tunnel.ipv4_tos;
> + out_tun_info->tunnel.ipv4_ttl = tun_info->tunnel.ipv4_ttl;
> + out_tun_info->tunnel.tun_flags = tun_info->tunnel.tun_flags;
> +
> + out_tun_info->tunnel.tp_src = tp_src;
> + out_tun_info->tunnel.tp_dst = tp_dst;
> +
> + /* clear struct padding. */
> + memset((unsigned char *) &tun_info->tunnel + OVS_TUNNEL_KEY_SIZE, 0,
> + sizeof(tun_info->tunnel) - OVS_TUNNEL_KEY_SIZE);
> +
> + out_tun_info->options = tun_info->options;
> + out_tun_info->options_len = tun_info->options_len;
> +}
> +
If you change ovs_flow_tun_info_init() according to comments above,
there is no need for separate function ovs_flow_out_tun_info_init().
You can use ovs_flow_tun_info_init() to setup out_tun_key.
>
> +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;
You can move this check inside ovs_vport_get_out_tun_info().
> +
> +
> +
extra blank lines.
> + /*
> + * 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));
> +
> +}
> +
.....
> }
> +
> +int ovs_vport_get_out_tun_info(struct ovs_tunnel_info *out_tun_info,
> + const struct ovs_tunnel_info *tun_info,
> + struct net *net,
> + u8 ipproto,
> + u32 skb_mark,
> + __be16 tp_src,
> + __be16 tp_dst)
> +{
> + struct rtable *rt;
...
> +
> + ip_rt_put(rt);
> +
> + /* Generate out_tun_info based on tun_info, saddr, tp_src and tp_dst*/
> + ovs_flow_out_tun_info_init(out_tun_info, tun_info, saddr, tp_src,
> tp_dst);
> +
Use tab for indentation.
> + return 0;
> +}
> diff --git a/datapath/vport.h b/datapath/vport.h
....
> - OVS_PACKET_ATTR_USERDATA, /* OVS_ACTION_ATTR_USERSPACE arg. */
> + OVS_PACKET_ATTR_PACKET, /* Packet data. */
> + OVS_PACKET_ATTR_KEY, /* Nested OVS_KEY_ATTR_* attributes.
> */
> + OVS_PACKET_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_*
> attributes. */
> + OVS_PACKET_ATTR_USERDATA, /* OVS_ACTION_ATTR_USERSPACE arg. */
> + OVS_PACKET_ATTR_OUT_TUNNEL_KEY, /* Nested OVS_TUNNEL_KEY_ATTR_*
> attributes */
> __OVS_PACKET_ATTR_MAX
> };
>
After reading patch I think "egress_tunnel_info" better than
out_tunnel_key, Can you change all names to use egress_tunnel_info
suffix?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
