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.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
