On Oct 10, 2013, at 2:56 PM, Romain Lenglet <rleng...@vmware.com> wrote:
> 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.
It would even be simpler to just disable IPFIX altogether with a warning log if
that action is not supported by the datapath.
>
>> {
>> 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
