On Oct 7, 2013, at 2:33 PM, Ben Pfaff <b...@nicira.com> wrote:
> This prevents using an older datapath from breaking forwarding.
>
> CC: Romain Lenglet <rleng...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif-ipfix.c | 28 ++++++++++---
> ofproto/ofproto-dpif-ipfix.h | 5 ++-
> ofproto/ofproto-dpif.c | 91 +++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 116 insertions(+), 8 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index a9cc73e..1a0ebae 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2012 Nicira, Inc.
> + * Copyright (c) 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -71,6 +71,7 @@ struct dpif_ipfix {
> struct dpif_ipfix_bridge_exporter bridge_exporter;
> struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_node. */
> atomic_int ref_cnt;
> + bool variable_length_userdata;
> };
>
> #define IPFIX_VERSION 0x000a
> @@ -632,14 +633,21 @@ dpif_ipfix_set_options(
> ovs_mutex_unlock(&mutex);
> }
>
> +/* Creates and returns a new dpif_ipfix structure.
> + *
> + * 'variable_length_userdata' specifies whether the datapath supports
> + * variable-length OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE
> + * actions. A datapath that doesn't support such actions cannot support flow
> + * sampling, so the module will suppress its use. */
> struct dpif_ipfix *
> -dpif_ipfix_create(void)
> +dpif_ipfix_create(bool variable_length_userdata)
> {
> struct dpif_ipfix *di;
> di = xzalloc(sizeof *di);
> dpif_ipfix_bridge_exporter_init(&di->bridge_exporter);
> hmap_init(&di->flow_exporter_map);
> atomic_init(&di->ref_cnt, 1);
> + di->variable_length_userdata = variable_length_userdata;
> return di;
> }
>
> @@ -1239,13 +1247,23 @@ ipfix_send_data_msg(struct dpif_ipfix_exporter
> *exporter,
> }
>
> static void
> -dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> +dpif_ipfix_sample(struct dpif_ipfix *di,
> + struct dpif_ipfix_exporter *exporter,
> struct ofpbuf *packet, const struct flow *flow,
> uint64_t packet_delta_count, uint32_t obs_domain_id,
> uint32_t obs_point_id)
The *_sample() functions are called only by upcalls triggered by the
NXAST_SAMPLE actions.
If the datapath doesn't support that action, there's none, so this function is
never called, so there's no need to do this check here.
Instead, I'd add this check into compose_ipfix_action() in
ofproto/ofproto-dpif-xlate.c, to call compose_sample_action() to add
NXAST_SAMPLE actions only if supported.
> {
> struct ipfix_flow_cache_entry *entry;
>
> + if (!di->variable_length_userdata) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +
> + VLOG_ERR_RL(&rl, "ignoring NXAST_SAMPLE action because datapath "
> + "lacks support (needs Linux 3.10+ or kernel module from "
> + "OVS 1.11+)");
> + return;
> + }
> +
> /* Create a flow cache entry from the sample. */
> entry = xmalloc(sizeof *entry);
> ipfix_cache_entry_init(entry, packet, flow, packet_delta_count,
> @@ -1263,7 +1281,7 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, struct
> ofpbuf *packet,
> /* Use the sampling probability as an approximation of the number
> * of matched packets. */
> packet_delta_count = UINT32_MAX / di->bridge_exporter.probability;
> - dpif_ipfix_sample(&di->bridge_exporter.exporter, packet, flow,
> + dpif_ipfix_sample(di, &di->bridge_exporter.exporter, packet, flow,
> packet_delta_count,
> di->bridge_exporter.options->obs_domain_id,
> di->bridge_exporter.options->obs_point_id);
> @@ -1284,7 +1302,7 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, struct
> ofpbuf *packet,
> ovs_mutex_lock(&mutex);
> node = dpif_ipfix_find_flow_exporter_map_node(di, collector_set_id);
> if (node) {
> - dpif_ipfix_sample(&node->exporter.exporter, packet, flow,
> + dpif_ipfix_sample(di, &node->exporter.exporter, packet, flow,
> packet_delta_count, obs_domain_id, obs_point_id);
> }
> ovs_mutex_unlock(&mutex);
> diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h
> index 6ebf8b0..a81976c 100644
> --- a/ofproto/ofproto-dpif-ipfix.h
> +++ b/ofproto/ofproto-dpif-ipfix.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2012 Nicira, Inc.
> + * Copyright (c) 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -17,6 +17,7 @@
> #ifndef OFPROTO_DPIF_IPFIX_H
> #define OFPROTO_DPIF_IPFIX_H 1
>
> +#include <stdbool.h>
> #include <stddef.h>
> #include <stdint.h>
>
> @@ -25,7 +26,7 @@ struct ofpbuf;
> struct ofproto_ipfix_bridge_exporter_options;
> struct ofproto_ipfix_flow_exporter_options;
>
> -struct dpif_ipfix *dpif_ipfix_create(void);
> +struct dpif_ipfix *dpif_ipfix_create(bool variable_length_userdata);
> struct dpif_ipfix *dpif_ipfix_ref(const struct dpif_ipfix *);
> void dpif_ipfix_unref(struct dpif_ipfix *);
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 80e97e0..e1a690b 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -41,6 +41,7 @@
> #include "netdev-vport.h"
> #include "netdev.h"
> #include "netlink.h"
> +#include "netlink-socket.h"
> #include "nx-match.h"
> #include "odp-util.h"
> #include "odp-execute.h"
> @@ -440,6 +441,10 @@ struct dpif_backer {
>
> /* Number of upcall handling threads. */
> unsigned int n_handler_threads;
> +
> + /* True if the datapath supports variable-length
> + * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. */
> + bool variable_length_userdata;
> };
>
> /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> @@ -1095,6 +1100,8 @@ struct odp_garbage {
> odp_port_t odp_port;
> };
>
> +static bool check_variable_length_userdata(struct dpif_backer *backer);
> +
> static int
> open_dpif_backer(const char *type, struct dpif_backer **backerp)
> {
> @@ -1197,6 +1204,7 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
> close_dpif_backer(backer);
> return error;
> }
> + backer->variable_length_userdata =
> check_variable_length_userdata(backer);
> udpif_recv_set(backer->udpif, n_handler_threads,
> backer->recv_set_enable);
> backer->n_handler_threads = n_handler_threads;
> @@ -1209,6 +1217,86 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
> return error;
> }
>
> +/* Tests whether 'backer''s datapath supports variable-length
> + * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. We need
> + * to disable some features on older datapaths that don't support this
> + * feature.
> + *
> + * Returns false if 'backer' definitely does not support variable-length
> + * userdata, true if it seems to support them or if at least the error we get
> + * is ambiguous. */
> +static bool
> +check_variable_length_userdata(struct dpif_backer *backer)
> +{
> + struct eth_header *eth;
> + struct ofpbuf actions;
> + struct ofpbuf key;
> + struct ofpbuf packet;
> + size_t start;
> + int error;
> +
> + /* Compose a userspace action that will cause an ERANGE error on older
> + * datapaths that don't support variable-length userdata.
> + *
> + * We really test for using userdata longer than 8 bytes, but older
> + * datapaths accepted these, silently truncating the userdata to 8 bytes.
> + * The same older datapaths rejected userdata shorter than 8 bytes, so we
> + * test for that instead as a proxy for longer userdata support. */
> + ofpbuf_init(&actions, 64);
> + start = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_USERSPACE);
> + nl_msg_put_u32(&actions, OVS_USERSPACE_ATTR_PID,
> + dpif_port_get_pid(backer->dpif, ODPP_NONE));
> + nl_msg_put_unspec_zero(&actions, OVS_USERSPACE_ATTR_USERDATA, 4);
> + nl_msg_end_nested(&actions, start);
> +
> + /* Compose an ODP flow key. The key is arbitrary but it must match the
> + * packet that we compose later. */
> + ofpbuf_init(&key, 64);
> + nl_msg_put_u32(&key, OVS_KEY_ATTR_IN_PORT, 0);
> + nl_msg_put_unspec_zero(&key, OVS_KEY_ATTR_ETHERNET,
> + sizeof(struct ovs_key_ethernet));
> + nl_msg_put_be16(&key, OVS_KEY_ATTR_ETHERTYPE, htons(0x1234));
> +
> + /* Compose a packet that matches the key. */
> + ofpbuf_init(&packet, ETH_HEADER_LEN);
> + eth = ofpbuf_put_zeros(&packet, ETH_HEADER_LEN);
> + eth->eth_type = htons(0x1234);
> +
> + /* Execute the actions. On older datapaths this fails with -ERANGE, on
> + * newer datapaths it succeeds. */
> + error = dpif_execute(backer->dpif, key.data, key.size,
> + actions.data, actions.size, &packet);
> +
> + ofpbuf_uninit(&packet);
> + ofpbuf_uninit(&key);
> + ofpbuf_uninit(&actions);
> +
> + switch (error) {
> + case 0:
> + /* Variable-length userdata is supported.
> + *
> + * Purge received packets to avoid processing the nonsense packet we
> + * sent to userspace, then report success. */
> + dpif_recv_purge(backer->dpif);
> + return true;
> +
> + case ERANGE:
> + /* Variable-length userdata is not supported. */
> + VLOG_WARN("%s: datapath does not support variable-length userdata "
> + "feature (needs Linux 3.10+ or kernel module from OVS "
> + "1..11+). The NXAST_SAMPLE action will be ignored.",
> + dpif_name(backer->dpif));
> + return false;
> +
> + default:
> + /* Something odd happened. We're not sure whether variable-length
> + * userdata is supported. Definitely to "yes". */
> + VLOG_WARN("%s: variable-length userdata feature probe failed (%s)",
> + dpif_name(backer->dpif), ovs_strerror(error));
> + return true;
> + }
> +}
> +
> static int
> construct(struct ofproto *ofproto_)
> {
> @@ -1881,7 +1969,8 @@ set_ipfix(
> bool has_options = bridge_exporter_options || flow_exporters_options;
>
> if (has_options && !di) {
> - di = ofproto->ipfix = dpif_ipfix_create();
> + di = ofproto->ipfix = dpif_ipfix_create(
> + ofproto->backer->variable_length_userdata);
> }
>
> if (di) {
> --
> 1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
