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.mc...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
