[ovs-dev] [patch] sFlow kernel code for Open vSwitch

[Neil McKee]

On Nov 20, 2009, at 2:10 PM, Ben Pfaff wrote:

> Neil McKee <neil.mckee at inmon.com> writes:
> 
>> Just one question: you'll also need to record the input
>> port+vlan+priority, and I don't think you can get that from the list
>> of actions.  Those fields are accessible in the struct odp_flow_key
>> that is passed to execute_actions().  Was there a reason to take that
>> part out?
> 
> Userspace can easily get at all of those.  The input port is in
> "struct odp_msg".  The input VLAN is in the packet itself, in the
> 802.1Q header.  By input priority do you mean the VLAN PCP bits?
> Those are also in the 802.1Q header.
> 

OK.  I guess if the input was assigned to a port-based-vlan (no 802.1Q tag) then that is also static information that does not change often and can be looked up in user-space.

>> other comments:
>> 
>> The sampling_probability thing seems elegant, although it seems like
>> it might add quite a few instructions to the fast-path for every
>> packet(?)  As an alternative, you could run a separate sampling
>> countdown on a per-CPU basis.  It looks like you have to access the
>> dp_stats_percpu structure anyway to safely increment the sample_pool
>> numbers, so a countdown-to-next-sample could be decremented there too.
>> If you have N CPUs all sampling *their* packets at 1-in-400 then their
>> combined effect is still to sample everything at 1-in-400.  Maybe
>> that's something to investigate later if we see sFlow having any
>> effect on performance.
> 
> You might be right about this, I will not dispute it.
> net_random() adds 44 instructions to the critical path here on
> i386 (although none of those are conditional branches).  An
> inline decrement-and-test would be maybe 10% of that length.  As
> you say, maybe I should put it back the way it was, but per-CPU.
> Any per-CPU method will be cheaper than any method that requires
> locking or atomic instructions here.

Seems OK to stick with net_random() for now.

> 
>> The sample_pool in the sFlow export is only a 32-bit counter, so you
>> don't need to use 64-bits for that (unless there is some other reason
>> for doing so).
> 
> I am slightly concerned about wraparound, so that userspace can
> properly reset 'sequence_number' each time the sample_pool wraps.
> But it does take several seconds for that to happen even with
> min-size packets at 10Gbps, so my concern may be pointless.
> Still, it has little cost.

It's OK for the sample_pool to wrap just like any other unsigned counter.   The collector should handle it.  You only need to reset the sequence numbers if the counters were reset abruptly for some reason -- which will probably never happen.

Neil