[ovs-dev] [PATCH] Change sFlow model to reflect per-bridge sampling

Ben Pfaff blp at nicira.com
Fri May 3 20:28:43 UTC 2013


Thanks, I did want your opinion.  I applied this to branch-1.10.

On Thu, May 02, 2013 at 05:19:42PM -0700, Neil Mckee wrote:
> Thanks!
> 
> If that was a question to me, then yes, I think it would be good to
> fix this on branch 1.10 too.  The old code resulted in ifIndex==-95
> being offered to the sFlow module for traffic from a
> non-ifindex-port.  -95 being the -errno that the netdev replies with
> to indicate that it doesn't have an ifindex.  That might have been
> tolerable except that every sub-agent (bridge) was claiming to
> represent this same bogus datasource, and that's what was violating
> the sFlow containment model.  Traffic on ifIndex-ports was still
> being reported OK, but non-ifindex-port traffic on different bridges
> (e.g. gre tunnel traffic) was being aliased together.  Given the
> importance of tunnel traffic...
> 
> But it's up to you.
> 
> Neil
> 
> 
> On May 2, 2013, at 12:38 PM, Ben Pfaff wrote:
> 
> > Applied to master and branch-1.11, thanks.  Do we need this on
> > branch-1.10 also?
> > 
> > On Tue, Apr 30, 2013 at 10:38:53PM -0700, Neil Mckee wrote:
> >> I learned how to squash patches...
> >> 
> >> Change sFlow model to reflect per-bridge sampling.  Before we were presenting a separate
> >> sFlow data-source (sampler) for each ifIndex-interface,  and it was causing 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 at inmon.com>
> >> 
> >> ---
> >> lib/sflow.h                  |  7 ++++
> >> ofproto/ofproto-dpif-sflow.c | 80 +++++++++++++++++++++-----------------------
> >> ofproto/ofproto-dpif.c       |  6 +++-
> >> ofproto/tunnel.c             |  1 -
> >> tests/ofproto-dpif.at        | 48 +++++++++++++-------------
> >> 5 files changed, 75 insertions(+), 67 deletions(-)
> >> 
> >> diff --git a/lib/sflow.h b/lib/sflow.h
> >> index 8ea9693..0d1f2b9 100644
> >> --- a/lib/sflow.h
> >> +++ b/lib/sflow.h
> >> @@ -8,6 +8,13 @@
> >> #ifndef SFLOW_H
> >> #define SFLOW_H 1
> >> 
> >> +typedef enum {
> >> +    SFL_DSCLASS_IFINDEX = 0,
> >> +    SFL_DSCLASS_VLAN = 1,
> >> +    SFL_DSCLASS_PHYSICAL_ENTITY = 2,
> >> +    SFL_DSCLASS_LOGICAL_ENTITY = 3
> >> +} SFL_DSCLASS;
> >> +
> >> enum SFLAddress_type {
> >>     SFLADDRESSTYPE_IP_V4 = 1,
> >>     SFLADDRESSTYPE_IP_V6 = 2
> >> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> >> index 69362ab..9ad0eaf 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 3ae3532..4306d40 100644
> >> --- a/ofproto/ofproto-dpif.c
> >> +++ b/ofproto/ofproto-dpif.c
> >> @@ -1782,7 +1782,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 8aa7fbe..8d29184 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);
> >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> >> index b1d2f26..b813fd0 100644
> >> --- a/tests/ofproto-dpif.at
> >> +++ b/tests/ofproto-dpif.at
> >> @@ -1252,7 +1252,7 @@ AT_CHECK([[sort sflow.log | $EGREP 'HEADER|ERROR' | sed 's/ /\
> >> 	/g']], [0], [dnl
> >> HEADER
> >> 	dgramSeqNo=1
> >> -	ds=127.0.0.1>0:1003
> >> +	ds=127.0.0.1>2:1000
> >> 	fsSeqNo=1
> >> 	in_vlan=0
> >> 	in_priority=0
> >> @@ -1261,7 +1261,7 @@ HEADER
> >> 	meanSkip=1
> >> 	samplePool=1
> >> 	dropEvents=0
> >> -	in_ifindex=1003
> >> +	in_ifindex=1004
> >> 	in_format=0
> >> 	out_ifindex=2
> >> 	out_format=2
> >> @@ -1269,10 +1269,10 @@ HEADER
> >> 	pkt_len=64
> >> 	stripped=4
> >> 	hdr_len=60
> >> -	hdr=FF-FF-FF-FF-FF-FF-50-54-00-00-00-07-08-06-00-01-08-00-06-04-00-01-50-54-00-00-00-07-C0-A8-00-01-00-00-00-00-00-00-C0-A8-00-02-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> +	hdr=FF-FF-FF-FF-FF-FF-50-54-00-00-00-05-08-06-00-01-08-00-06-04-00-01-50-54-00-00-00-05-C0-A8-00-02-00-00-00-00-00-00-C0-A8-00-01-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> HEADER
> >> 	dgramSeqNo=1
> >> -	ds=127.0.0.1>0:1003
> >> +	ds=127.0.0.1>2:1000
> >> 	fsSeqNo=2
> >> 	in_vlan=0
> >> 	in_priority=0
> >> @@ -1283,16 +1283,16 @@ HEADER
> >> 	dropEvents=0
> >> 	in_ifindex=1003
> >> 	in_format=0
> >> -	out_ifindex=1004
> >> -	out_format=0
> >> +	out_ifindex=2
> >> +	out_format=2
> >> 	hdr_prot=1
> >> 	pkt_len=64
> >> 	stripped=4
> >> 	hdr_len=60
> >> -	hdr=50-54-00-00-00-05-50-54-00-00-00-07-08-00-45-00-00-1C-00-00-00-00-40-01-F9-8D-C0-A8-00-02-C0-A8-00-01-00-00-FF-FF-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> +	hdr=FF-FF-FF-FF-FF-FF-50-54-00-00-00-07-08-06-00-01-08-00-06-04-00-01-50-54-00-00-00-07-C0-A8-00-01-00-00-00-00-00-00-C0-A8-00-02-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> HEADER
> >> 	dgramSeqNo=1
> >> -	ds=127.0.0.1>0:1003
> >> +	ds=127.0.0.1>2:1000
> >> 	fsSeqNo=3
> >> 	in_vlan=0
> >> 	in_priority=0
> >> @@ -1301,55 +1301,55 @@ HEADER
> >> 	meanSkip=1
> >> 	samplePool=3
> >> 	dropEvents=0
> >> -	in_ifindex=1003
> >> +	in_ifindex=1004
> >> 	in_format=0
> >> -	out_ifindex=1004
> >> +	out_ifindex=1003
> >> 	out_format=0
> >> 	hdr_prot=1
> >> 	pkt_len=64
> >> 	stripped=4
> >> 	hdr_len=60
> >> -	hdr=50-54-00-00-00-05-50-54-00-00-00-07-86-DD-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> +	hdr=50-54-00-00-00-07-50-54-00-00-00-05-08-00-45-00-00-1C-00-00-00-00-40-01-F9-8D-C0-A8-00-01-C0-A8-00-02-08-00-F7-FF-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> HEADER
> >> 	dgramSeqNo=1
> >> -	ds=127.0.0.1>0:1004
> >> -	fsSeqNo=1
> >> +	ds=127.0.0.1>2:1000
> >> +	fsSeqNo=4
> >> 	in_vlan=0
> >> 	in_priority=0
> >> 	out_vlan=0
> >> 	out_priority=0
> >> 	meanSkip=1
> >> -	samplePool=1
> >> +	samplePool=4
> >> 	dropEvents=0
> >> -	in_ifindex=1004
> >> +	in_ifindex=1003
> >> 	in_format=0
> >> -	out_ifindex=2
> >> -	out_format=2
> >> +	out_ifindex=1004
> >> +	out_format=0
> >> 	hdr_prot=1
> >> 	pkt_len=64
> >> 	stripped=4
> >> 	hdr_len=60
> >> -	hdr=FF-FF-FF-FF-FF-FF-50-54-00-00-00-05-08-06-00-01-08-00-06-04-00-01-50-54-00-00-00-05-C0-A8-00-02-00-00-00-00-00-00-C0-A8-00-01-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> +	hdr=50-54-00-00-00-05-50-54-00-00-00-07-08-00-45-00-00-1C-00-00-00-00-40-01-F9-8D-C0-A8-00-02-C0-A8-00-01-00-00-FF-FF-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> HEADER
> >> 	dgramSeqNo=1
> >> -	ds=127.0.0.1>0:1004
> >> -	fsSeqNo=2
> >> +	ds=127.0.0.1>2:1000
> >> +	fsSeqNo=5
> >> 	in_vlan=0
> >> 	in_priority=0
> >> 	out_vlan=0
> >> 	out_priority=0
> >> 	meanSkip=1
> >> -	samplePool=2
> >> +	samplePool=5
> >> 	dropEvents=0
> >> -	in_ifindex=1004
> >> +	in_ifindex=1003
> >> 	in_format=0
> >> -	out_ifindex=1003
> >> +	out_ifindex=1004
> >> 	out_format=0
> >> 	hdr_prot=1
> >> 	pkt_len=64
> >> 	stripped=4
> >> 	hdr_len=60
> >> -	hdr=50-54-00-00-00-07-50-54-00-00-00-05-08-00-45-00-00-1C-00-00-00-00-40-01-F9-8D-C0-A8-00-01-C0-A8-00-02-08-00-F7-FF-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> +	hdr=50-54-00-00-00-05-50-54-00-00-00-07-86-DD-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> >> ])
> >> 
> >> AT_CHECK([[sort sflow.log | $EGREP 'IFCOUNTERS|ERROR' | head -6 | sed 's/ /\
> >> -- 
> >> 1.8.1.4
> >> 
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev
> 



More information about the dev mailing list