OK, I can resubmit the patch with something like this in
ofproto/ofproto-dpif.c:port_construct():
if (netdev_vport_is_patch(netdev)) {
/* 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;
}
but I'm not sure I understand the one in tunnel.c. I assume you mean this
comment:
/* XXX:
*
* Ability to generate metadata for packet-outs
* Disallow netdevs with names like "gre64_system" to prevent collisions. */
Is this referring to the code that appears 60 lines later? i.e. this test
here:
existing_port = tnl_find_exact(&tnl_port->match);
if (existing_port) {
if (warn) {
struct ds ds = DS_EMPTY_INITIALIZER;
tnl_match_fmt(&tnl_port->match, &ds);
VLOG_WARN("%s: attempting to add tunnel port with same config as "
"port '%s' (%s)", tnl_port_get_name(tnl_port),
tnl_port_get_name(existing_port), ds_cstr(&ds));
ds_destroy(&ds);
free(tnl_port);
}
return &void_tnl_port;
}
And is the concern related to filling in the output-port correctly in sFlow
samples? Using that scheme where the output-port is stored in a per-flow
cookie and retrieved later when a sampl e is taken? If I have understood
correctly, you might have "collisions" where the tunnel netdev name is the
same for two tunnels even if the ultimate datapath output port is going to be
different. And so the revised comment should explain that if that happens the
sFlow output port is not going to be filled in, and will instead be reported
as either 0=="unknown" or 0x80000001=="one interface". Did I get that right?
Neil
On Apr 29, 2013, at 11:39 AM, Jesse Gross wrote:
> OK, that sounds reasonable to me. Can you update the comments to
> reflect that sFlow should be OK now - assuming that you think it is. I
> see two: ofproto/ofproto-dpif.c:port_construct() and the one in
> ofproto/tunnel.c about metadata for packet outs, which was intended to
> be for sFlow.
>
> On Thu, Apr 25, 2013 at 8:18 PM, Neil McKee <neil.mc...@inmon.com> wrote:
>> Thank you for spending time thinking about this.
>>
>> There is no loss of information in changing from a per-interface sampling
>> model to a per-bridge sampling model.
>>
>> If there is a difference it's only on the configuration side, and since the
>> OVS sFlow config is already per-bridge, there is no conflict there.
>>
>> Hypothetically, if the datapath offered separate per-physical-port sampling
>> probability and we always knew what physical port a packet came in/went out
>> on, then we would have the choice of staying with the per-interface model
>> and changing the config schema to expose it. However that seems like a
>> stretch -- and not worth the effort at all if you remember that the normal
>> practice for sFlow monitoring is to configure the same sampling rate on
>> every port of every switch in the LAN.
>>
>> Neil
>>
>>
>>
>> On Apr 25, 2013, at 5:06 PM, Jesse Gross <je...@nicira.com> wrote:
>>
>>> OK, thanks for the explanation. I guess the part that I'm not entirely
>>> sure about is what is the tradeoff in modeling this as being
>>> per-bridge rather than per-port? It seems like like in the case of
>>> per-bridge sampling you are still including the ifIndex with the
>>> sample metadata so you don't lose any information. Is there any
>>> disadvantage?
>>>
>>> On Wed, Apr 24, 2013 at 2:04 PM, Neil Mckee <neil.mc...@inmon.com> wrote:
>>>> Actually it wasn't just tunnel ports. Anything that didn't have an
>>>> ifIndex was mapping to the same sFlow datasource. Even if it was on
>>>> another bridge.
>>>>
>>>> Besides, sFlow wouldn't want to model every tunnel as a separate
>>>> datasource either -- even if they did have ifIndex numbers. There might
>>>> be thousands. The idea is to have a small number of fairly-persistent
>>>> datasources and report on tunnels by annotating individual packet samples
>>>> (see below).
>>>>
>>>> I think the original thinking (years ago) was that we might ignore all
>>>> samples that did not ingress on an ifindex-interface, but that's not what
>>>> is happening, and it's not what we want to happen now anyway. Now it's
>>>> quite common to see ingress-sampled packets that we really want to report
>>>> on where that ingress port doesn't have an ifIndex (GRE tunnels being just
>>>> the most obvious example). So if we drop the per-interface-sampler
>>>> representation and adopt one that more accurately represents what is going
>>>> on below then it falls out correctly, and still conforms to the sFlow v5
>>>> standard.
>>>>
>>>> So looking at the code, the hash-table that the ofproto-sflow module
>>>> maintains to look up odp_port -> struct of_port becomes just a cache of
>>>> the ifindex ports. It is still needed so that the counter-push
>>>> data-sources can retrieve the latest stats from the netdev, and so that
>>>> the packet samples can have their ingress ifindex filled in where
>>>> applicable. However if an ingress sample comes in for some other port
>>>> and there is no lookup in the table then it's no longer a show-stopper.
>>>> It just means that the packet sample goes out with inputPort = 0 (meaning
>>>> "ifindex unknown ").
>>>>
>>>> Ideally, we would want to know the original ifIndex of the physical port
>>>> where that tunnel first entered the hypervisor, but that seems hard to
>>>> know.
>>>>
>>>> Looking ahead, it would be good to know the details of the tunnel too,
>>>> so we could annotate the packet-sample with another structure:
>>>> http://sflow.org/sflow_tunnels.txt
>>>>
>>>> but that might be part of a broader discussion where we look at ways to
>>>> expose more state to the ofproto-sflow module. For example, I am
>>>> currently looking at how the bundle information might be exposed so that
>>>> we can also fill in these structures:
>>>> http://www.sflow.org/sflow_lag.txt
>>>>
>>>> And sFlow has standard structures for reporting on MPLS too:
>>>> http://www.sflow.org/SFLOW-STRUCTS5.txt
>>>>
>>>> It seems like there is choice between (1) pushing the information down to
>>>> ofproto-sflow on a state-change (as we do with that odp_port -> of_port
>>>> lookup) or (2) providing callback hooks that allow ofproto-dpif-sflow to
>>>> ask ofproto-dpif questions about tunnels, bundles and more. Right now it
>>>> looks to me like the latter would be better, because it allows
>>>> ofproto-dpif to completely control what is exposed and keep a tight grip
>>>> on all it's internal pointers. But maybe there are other architectural
>>>> considerations?
>>>>
>>>> Neil
>>>>
>>>>
>>>> On Apr 24, 2013, at 11:09 AM, Jesse Gross wrote:
>>>>
>>>>> On Wed, Apr 24, 2013 at 10:46 AM, Neil Mckee <neil.mc...@inmon.com> wrote:
>>>>>> Adjust sFlow packet samplers to better reflect the underlying per-bridge
>>>>>> sampling that is really happening, rather than modeling it as
>>>>>> per-interface
>>>>>> sampling. This fixes an aliasing issue: tunnel traffic sampled on
>>>>>> different
>>>>>> bridges was all being reported against the same sFlow datasource.
>>>>>>
>>>>>> The sFlow counter samples remain unchanged, reporting only on interfaces
>>>>>> that have valid ifIndex numbers.
>>>>>>
>>>>>> See definition of SFlowDataSource in
>>>>>> http://sflow.org/sflow_version_5.txt.
>>>>>>
>>>>>> Signed-off-by: Neil McKee <neil.mc...@inmon.com>
>>>>>
>>>>> It actually is possible to disambiguate between tunnel ports in order
>>>>> to continue reporting on a per-port basis, have you considered doing
>>>>> that?
>>>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
