Change sFlow model to reflect per-bridge sampling

Until now, we were presenting a separate sFlow data-source (sampler) for
each ifIndex-interface.  This caused problems with samples that did not
easily map to an ifIndex being aliased together and breaking the sFlow
containment rules.  This patch changes the model to present a single sFlow
data-source for each bridge.  Now we can still make all reasonable effort
to map packet samples to ingress/egress ifIndex numbers, knowing that the
fallback to "unknown" does not break the sFlow model.  Note that
interface-counter-polling is still handled the same way as before, with
sFlow counter-polling data only being exported for ifIndex-interfaces.

Signed-off-by: Neil Mckee <neil.mckee@inmon.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

3 files changed, 44 insertions, 43 deletions

diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 69362ab8..9ad0eaf2 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -341,39 +341,32 @@ dpif_sflow_add_poller(struct dpif_sflow *ds, struct dpif_sflow_port *dsp)
     sfl_poller_set_bridgePort(poller, dsp->odp_port);
 }
 
-static void
-dpif_sflow_add_sampler(struct dpif_sflow *ds, struct dpif_sflow_port *dsp)
-{
-    SFLSampler *sampler = sfl_agent_addSampler(ds->sflow_agent, &dsp->dsi);
-    sfl_sampler_set_sFlowFsPacketSamplingRate(sampler, ds->options->sampling_rate);
-    sfl_sampler_set_sFlowFsMaximumHeaderSize(sampler, ds->options->header_len);
-    sfl_sampler_set_sFlowFsReceiver(sampler, RECEIVER_INDEX);
-}
-
 void
 dpif_sflow_add_port(struct dpif_sflow *ds, struct ofport *ofport,
                     uint32_t odp_port)
 {
     struct dpif_sflow_port *dsp;
-    uint32_t ifindex;
+    int ifindex;
 
     dpif_sflow_del_port(ds, odp_port);
 
-    /* Add to table of ports. */
-    dsp = xmalloc(sizeof *dsp);
     ifindex = netdev_get_ifindex(ofport->netdev);
+
     if (ifindex <= 0) {
-        ifindex = (ds->sflow_agent->subId << 16) + odp_port;
+        /* Not an ifindex port, so do not add a cross-reference to it here */
+        return;
     }
+
+    /* Add to table of ports. */
+    dsp = xmalloc(sizeof *dsp);
     dsp->ofport = ofport;
     dsp->odp_port = odp_port;
-    SFL_DS_SET(dsp->dsi, 0, ifindex, 0);
+    SFL_DS_SET(dsp->dsi, SFL_DSCLASS_IFINDEX, ifindex, 0);
     hmap_insert(&ds->ports, &dsp->hmap_node, hash_int(odp_port, 0));
 
-    /* Add poller and sampler. */
+    /* Add poller. */
     if (ds->sflow_agent) {
         dpif_sflow_add_poller(ds, dsp);
-        dpif_sflow_add_sampler(ds, dsp);
     }
 }
 
@@ -406,6 +399,9 @@ dpif_sflow_set_options(struct dpif_sflow *ds,
     SFLReceiver *receiver;
     SFLAddress agentIP;
     time_t now;
+    SFLDataSource_instance dsi;
+    uint32_t dsIndex;
+    SFLSampler *sampler;
 
     if (sset_is_empty(&options->targets) || !options->sampling_rate) {
         /* No point in doing any work if there are no targets or nothing to
@@ -473,10 +469,20 @@ dpif_sflow_set_options(struct dpif_sflow *ds,
     /* Set the sampling_rate down in the datapath. */
     ds->probability = MAX(1, UINT32_MAX / ds->options->sampling_rate);
 
-    /* Add samplers and pollers for the currently known ports. */
+    /* Add a single sampler for the bridge. This appears as a PHYSICAL_ENTITY
+       because it is associated with the hypervisor, and interacts with the server
+       hardware directly.  The sub_id is used to distinguish this sampler from
+       others on other bridges within the same agent. */
+    dsIndex = 1000 + options->sub_id;
+    SFL_DS_SET(dsi, SFL_DSCLASS_PHYSICAL_ENTITY, dsIndex, 0);
+    sampler = sfl_agent_addSampler(ds->sflow_agent, &dsi);
+    sfl_sampler_set_sFlowFsPacketSamplingRate(sampler, ds->options->sampling_rate);
+    sfl_sampler_set_sFlowFsMaximumHeaderSize(sampler, ds->options->header_len);
+    sfl_sampler_set_sFlowFsReceiver(sampler, RECEIVER_INDEX);
+
+    /* Add pollers for the currently known ifindex-ports */
     HMAP_FOR_EACH (dsp, hmap_node, &ds->ports) {
         dpif_sflow_add_poller(ds, dsp);
-        dpif_sflow_add_sampler(ds, dsp);
     }
 }
 
@@ -499,43 +505,35 @@ dpif_sflow_received(struct dpif_sflow *ds, struct ofpbuf *packet,
     SFLFlow_sample_element switchElem;
     SFLSampler *sampler;
     struct dpif_sflow_port *in_dsp;
-    struct netdev_stats stats;
     ovs_be16 vlan_tci;
-    int error;
 
-    /* Build a flow sample */
-    memset(&fs, 0, sizeof fs);
-
-    in_dsp = dpif_sflow_find_port(ds, odp_in_port);
-    if (!in_dsp) {
+    sampler = ds->sflow_agent->samplers;
+    if (!sampler) {
         return;
     }
-    fs.input = SFL_DS_INDEX(in_dsp->dsi);
 
-    error = ofproto_port_get_stats(in_dsp->ofport, &stats);
-    if (error) {
-        VLOG_WARN_RL(&rl, "netdev get-stats error %s", strerror(error));
-        return;
-    }
-    fs.sample_pool = stats.rx_packets;
+    /* Build a flow sample. */
+    memset(&fs, 0, sizeof fs);
 
-    /* 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(ds->sflow_agent, fs.input);
-    if (!sampler) {
-        VLOG_WARN_RL(&rl, "no sampler for input ifIndex (%"PRIu32")",
-                     fs.input);
-        return;
+    /* Look up the input ifIndex if this port has one. Otherwise just
+     * leave it as 0 (meaning 'unknown') and continue. */
+    in_dsp = dpif_sflow_find_port(ds, odp_in_port);
+    if (in_dsp) {
+        fs.input = SFL_DS_INDEX(in_dsp->dsi);
     }
 
+    /* Make the assumption that the random number generator in the datapath converges
+     * to the configured mean, and just increment the samplePool by the configured
+     * sampling rate every time. */
+    sampler->samplePool += sfl_sampler_get_sFlowFsPacketSamplingRate(sampler);
+
     /* Sampled header. */
     memset(&hdrElem, 0, sizeof hdrElem);
     hdrElem.tag = SFLFLOW_HEADER;
     header = &hdrElem.flowType.header;
     header->header_protocol = SFLHEADER_ETHERNET_ISO8023;
     /* The frame_length should include the Ethernet FCS (4 bytes),
-       but it has already been stripped,  so we need to add 4 here. */
+     * but it has already been stripped,  so we need to add 4 here. */
     header->frame_length = packet->size + 4;
     /* Ethernet FCS stripped off. */
     header->stripped = 4;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 239f8d36..4bc66158 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1776,7 +1776,11 @@ port_construct(struct ofport *port_)
     port->carrier_seq = netdev_get_carrier_resets(netdev);
 
     if (netdev_vport_is_patch(netdev)) {
-        /* XXX By bailing out here, we don't do required sFlow work. */
+        /* By bailing out here, we don't submit the port to the sFlow module
+	 * to be considered for counter polling export.  This is correct
+	 * because the patch port represents an interface that sFlow considers
+	 * to be "internal" to the switch as a whole, and therefore not an
+	 * candidate for counter polling. */
         port->odp_port = OVSP_NONE;
         return 0;
     }
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index 8aa7fbea..8d29184f 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -32,7 +32,6 @@
 
 /* XXX:
  *
- * Ability to generate metadata for packet-outs
  * Disallow netdevs with names like "gre64_system" to prevent collisions. */
 
 VLOG_DEFINE_THIS_MODULE(tunnel);