From: Ben Pfaff Date: Wed, 20 Jan 2010 21:52:42 +0000 (-0800) Subject: sflow: Fix sFlow sampling structure. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=56fd8edf80b6098289f9ddd94a6a4be3be648472;p=openvswitch sflow: Fix sFlow sampling structure. According to Neil McKee, in an email archived at http://openvswitch.org/pipermail/dev_openvswitch.org/2010-January/000934.html: The containment rule is that a given sflow-datasource (sampler or poller) should be scoped within only one sflow-agent (or sub-agent). So the issue arrises when you have two switches/datapaths defined on the same host being managed with the same IP address: each switch is a separate sub-agent, so they can run independently (e.g. with their own sequence numbers) but they can't both claim to speak for the same sflow-datasource. Specifically, they can't both represent the :0 data-source. This containment rule is necessary so that the sFlow collector can scale and combine the results accurately. One option would be to stick with the :0 data-source but elevate it to be global across all bridges, with a global sample_pool and a global sflow_agent. Not tempting. Better to go the other way and allow each interface to have it's own sampler, just as it already has it's own poller. The ifIndex numbers are globally unique across all switches/datapaths on the host, so the containment is now clean. Datasource :5 might be on one switch, whille :7 can be on another. Other benefits are that 1) you can support the option of overriding the default sampling-rate on an interface-by-interface basis, and 2) this is how most sFlow implementations are coded, so there will be no surprises or interoperability issues with any sFlow collectors out there. This commit implements the approach suggested by Neil. This commit uses an atomic_t to represent the sampling pool. This is because we do want access to it to be atomic, but we expect that it will "mostly" be accessed from a single CPU at a time. Perhaps this is a bad assumption; we can always switch to another form of synchronization later. CC: Neil McKee --- diff --git a/datapath/actions.c b/datapath/actions.c index 7c618ccd..8b32de47 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -1,6 +1,6 @@ /* * Distributed under the terms of the GNU GPL version 2. - * Copyright (c) 2007, 2008, 2009 Nicira Networks. + * Copyright (c) 2007, 2008, 2009, 2010 Nicira Networks. * * Significant portions of this file may be copied from parts of the Linux * kernel, by Linus Torvalds and others. @@ -369,13 +369,13 @@ output_control(struct datapath *dp, struct sk_buff *skb, u32 arg, gfp_t gfp) /* Send a copy of this packet up to the sFlow agent, along with extra * information about what happened to it. */ static void sflow_sample(struct datapath *dp, struct sk_buff *skb, - const union odp_action *a, int n_actions, gfp_t gfp) + const union odp_action *a, int n_actions, + gfp_t gfp, struct net_bridge_port *nbp) { struct odp_sflow_sample_header *hdr; unsigned int actlen = n_actions * sizeof(union odp_action); unsigned int hdrlen = sizeof(struct odp_sflow_sample_header); struct sk_buff *nskb; - int i; nskb = skb_copy_expand(skb, actlen + hdrlen, 0, gfp); if (!nskb) @@ -384,12 +384,7 @@ static void sflow_sample(struct datapath *dp, struct sk_buff *skb, memcpy(__skb_push(nskb, actlen), a, actlen); hdr = (struct odp_sflow_sample_header*)__skb_push(nskb, hdrlen); hdr->n_actions = n_actions; - hdr->sample_pool = 0; - for_each_possible_cpu (i) { - const struct dp_stats_percpu *stats; - stats = per_cpu_ptr(dp->stats_percpu, i); - hdr->sample_pool += stats->sflow_pool; - } + hdr->sample_pool = atomic_read(&nbp->sflow_pool); dp_output_control(dp, nskb, _ODPL_SFLOW_NR, 0); } @@ -407,15 +402,13 @@ int execute_actions(struct datapath *dp, struct sk_buff *skb, int err; if (dp->sflow_probability) { - /* Increment sample pool. */ - int cpu = get_cpu(); - per_cpu_ptr(dp->stats_percpu, cpu)->sflow_pool++; - put_cpu(); - - /* Sample packet. */ - if (dp->sflow_probability == UINT_MAX || - net_random() < dp->sflow_probability) - sflow_sample(dp, skb, a, n_actions, gfp); + struct net_bridge_port *p = skb->dev->br_port; + if (p) { + atomic_inc(&p->sflow_pool); + if (dp->sflow_probability == UINT_MAX || + net_random() < dp->sflow_probability) + sflow_sample(dp, skb, a, n_actions, gfp, p); + } } for (; n_actions > 0; a++, n_actions--) { diff --git a/datapath/datapath.c b/datapath/datapath.c index f6a02f7a..760e524e 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007, 2008, 2009 Nicira Networks. + * Copyright (c) 2007, 2008, 2009, 2010 Nicira Networks. * Distributed under the terms of the GNU GPL version 2. * * Significant portions of this file may be copied from parts of the Linux @@ -349,6 +349,7 @@ static int new_nbp(struct datapath *dp, struct net_device *dev, int port_no) p->port_no = port_no; p->dp = dp; p->dev = dev; + atomic_set(&p->sflow_pool, 0); if (!is_dp_dev(dev)) rcu_assign_pointer(dev->br_port, p); else { diff --git a/datapath/datapath.h b/datapath/datapath.h index 7fa90f3c..d98a9656 100644 --- a/datapath/datapath.h +++ b/datapath/datapath.h @@ -94,15 +94,12 @@ struct dp_bucket { * @n_lost: Number of received packets that had no matching flow in the flow * table that could not be sent to userspace (normally due to an overflow in * one of the datapath's queues). - * @sflow_pool: Number of packets that were candidates for sFlow sampling, - * regardless of whether they were actually chosen and sent down to userspace. */ struct dp_stats_percpu { u64 n_frags; u64 n_hit; u64 n_missed; u64 n_lost; - u64 sflow_pool; }; struct dp_port_group { @@ -115,7 +112,7 @@ struct dp_port_group { * struct datapath - datapath for flow-based packet switching * @mutex: Mutual exclusion for ioctls. * @dp_idx: Datapath number (index into the dps[] array in datapath.c). - * @ifobj: &struct kobject representing the datapath. + * @ifobj: Represents /sys/class/net//brif. * @drop_frags: Drop all IP fragments if nonzero. * @queues: %DP_N_QUEUES sets of queued packets for userspace to handle. * @waitqueue: Waitqueue, for waiting for new packets in @queues. @@ -161,13 +158,28 @@ struct datapath { unsigned int sflow_probability; }; +/** + * struct net_bridge_port - one port within a datapath + * @port_no: Index into @dp's @ports array. + * @dp: Datapath to which this port belongs. + * @dev: The network device attached to this port. The @br_port member in @dev + * points back to this &struct net_bridge_port. + * @kobj: Represents /sys/class/net//brport. + * @linkname: The name of the link from /sys/class/net//brif to this + * &struct net_bridge_port. (We keep this around so that we can delete it + * if @dev gets renamed.) Set to the null string when no link exists. + * @node: Element in @dp's @port_list. + * @sflow_pool: Number of packets that were candidates for sFlow sampling, + * regardless of whether they were actually chosen and sent down to userspace. + */ struct net_bridge_port { u16 port_no; struct datapath *dp; struct net_device *dev; struct kobject kobj; char linkname[IFNAMSIZ]; - struct list_head node; /* Element in datapath.ports. */ + struct list_head node; + atomic_t sflow_pool; }; extern struct notifier_block dp_device_notifier; diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h index 6d3e9007..b079f529 100644 --- a/include/openvswitch/datapath-protocol.h +++ b/include/openvswitch/datapath-protocol.h @@ -150,17 +150,14 @@ struct odp_msg { * @sample_pool: Number of packets that were candidates for sFlow sampling, * regardless of whether they were actually chosen and sent down to userspace. * @n_actions: Number of "union odp_action"s immediately following this header. - * @reserved: Pads the structure up to a 64-bit boundary. Should be set to - * zero. * * This header follows &struct odp_msg when that structure's @type is * %_ODPL_SFLOW_NR, and it is itself followed by an array of &union odp_action * (the number of which is specified in @n_actions) and then by packet data. */ struct odp_sflow_sample_header { - __u64 sample_pool; + __u32 sample_pool; __u32 n_actions; - __u32 reserved; }; #define ODP_PORT_INTERNAL (1 << 0) /* This port is simulated. */ diff --git a/ofproto/ofproto-sflow.c b/ofproto/ofproto-sflow.c index aa91460f..b37db42d 100644 --- a/ofproto/ofproto-sflow.c +++ b/ofproto/ofproto-sflow.c @@ -296,6 +296,17 @@ ofproto_sflow_add_poller(struct ofproto_sflow *os, sfl_poller_set_bridgePort(poller, odp_port); } +static void +ofproto_sflow_add_sampler(struct ofproto_sflow *os, + struct ofproto_sflow_port *osp, + u_int32_t sampling_rate, u_int32_t header_len) +{ + SFLSampler *sampler = sfl_agent_addSampler(os->sflow_agent, &osp->dsi); + sfl_sampler_set_sFlowFsPacketSamplingRate(sampler, sampling_rate); + sfl_sampler_set_sFlowFsMaximumHeaderSize(sampler, header_len); + sfl_sampler_set_sFlowFsReceiver(sampler, RECEIVER_INDEX); +} + void ofproto_sflow_add_port(struct ofproto_sflow *os, uint16_t odp_port, const char *netdev_name) @@ -338,6 +349,7 @@ ofproto_sflow_del_port(struct ofproto_sflow *os, uint16_t odp_port) if (osp) { if (os->sflow_agent) { sfl_agent_removePoller(os->sflow_agent, &osp->dsi); + sfl_agent_removeSampler(os->sflow_agent, &osp->dsi); } netdev_close(osp->netdev); free(osp); @@ -350,9 +362,7 @@ ofproto_sflow_set_options(struct ofproto_sflow *os, const struct ofproto_sflow_options *options) { struct ofproto_sflow_port *osp; - SFLDataSource_instance dsi; bool options_changed; - SFLSampler *sampler; SFLReceiver *receiver; unsigned int odp_port; SFLAddress agentIP; @@ -421,27 +431,14 @@ ofproto_sflow_set_options(struct ofproto_sflow *os, sfl_receiver_set_sFlowRcvrOwner(receiver, "Open vSwitch sFlow"); sfl_receiver_set_sFlowRcvrTimeout(receiver, 0xffffffff); - /* Add a single sampler to represent the datapath (special :0 - * datasource). The alternative is to model a physical switch more closely - * and instantiate a separate sampler object for each interface, but then - * unicasts would have to be offered to two samplers, and - * broadcasts/multicasts would have to be offered to all of them. Doing it - * this way with a single :0 sampler is much more efficient for a - * virtual switch, and is allowed by the sFlow standard. - */ - SFL_DS_SET(dsi, 0, 0, 0); - sampler = sfl_agent_addSampler(os->sflow_agent, &dsi); - sfl_sampler_set_sFlowFsReceiver(sampler, RECEIVER_INDEX); - sfl_sampler_set_sFlowFsPacketSamplingRate(sampler, options->sampling_rate); - sfl_sampler_set_sFlowFsMaximumHeaderSize(sampler, options->header_len); - /* Set the sampling_rate down in the datapath. */ dpif_set_sflow_probability(os->dpif, MAX(1, UINT32_MAX / options->sampling_rate)); - /* Add the currently known ports. */ + /* Add samplers and pollers for the currently known ports. */ PORT_ARRAY_FOR_EACH (osp, &os->ports, odp_port) { - ofproto_sflow_add_poller(os, osp, odp_port); + ofproto_sflow_add_sampler(os, osp, + options->sampling_rate, options->header_len); } } @@ -460,7 +457,7 @@ ofproto_sflow_received(struct ofproto_sflow *os, struct odp_msg *msg) SFLFlow_sample_element hdrElem; SFLSampled_header *header; SFLFlow_sample_element switchElem; - SFLSampler *sampler = os->sflow_agent->samplers; + SFLSampler *sampler; const struct odp_sflow_sample_header *hdr; const union odp_action *actions; struct ofpbuf payload; @@ -505,6 +502,16 @@ ofproto_sflow_received(struct ofproto_sflow *os, struct odp_msg *msg) fs.output = 0; /* Filled in correctly below. */ fs.sample_pool = hdr->sample_pool; + /* We are going to give it to the sampler that represents this input port. + * By implementing "ingress-only" sampling like this we ensure that we + * never have to offer the same sample to more than one sampler. */ + sampler = sfl_agent_getSamplerByIfIndex(os->sflow_agent, fs.input); + if (!sampler) { + VLOG_WARN_RL(&rl, "no sampler for input ifIndex (%"PRIu32")", + fs.input); + return; + } + /* Sampled header. */ memset(&hdrElem, 0, sizeof hdrElem); hdrElem.tag = SFLFLOW_HEADER;