[ovs-dev] [PATCH v3] Genericize/simplify kernel sFlow implementation

[Neil McKee]

On Sep 22, 2011, at 4:07 PM, Pravin Shelar wrote:

> On Thu, Sep 22, 2011 at 1:56 PM, Ben Pfaff <blp at nicira.com> wrote:
>> On Wed, Sep 21, 2011 at 03:21:46PM -0700, Pravin Shelar wrote:
>>> v3: Fixed according to comments from Ben
>>>     -  Probability calculation
>>>     -  Coding style fixes
>>>     -  Now SAMPLE actions is not optional
>>>     -  Using offset to fix user-action-cookie.
>>> 
>>> I will post another patch to handle mirror output port case for sFlow.
>> 
>> "git am" says:
>> 
>> /home/blp/db/.git/rebase-apply/patch:249: space before tab in indent.
>>                 * OVS_ACTION_ATTR_SAMPLE, as it has nested actions list which
>> /home/blp/db/.git/rebase-apply/patch:250: space before tab in indent.
>>                 * is variable size. */
>> warning: 2 lines add whitespace errors.
> ok.
> 
>> 
>> I didn't really read the kernel changes.
>> 
>> 
>> It's not so great to have to call netdev_get_stats() for every sflow
>> packet we send.  Maybe we could rate-limit the calls and cache old
>> values?
> 
> OK, I will keep 1 sec interval for get stats.

Most of the counters only need to be read when you send sFlow COUNTER samples,  and that only happens about once every 30 seconds or so.   However when they are sent they need to be fresh,  otherwise you get re-sampling errors at the collector and the accuracy is compromised.    Presumably it's OK to call netdev_get_stats() once every 30 seconds for each interface?  (Each interface is put on a different schedule so that the calls are spread out in time).

The sending of PACKET samples is a different matter.   Is that what this is about?  I wasn't sure from the context.  If this is about including the sample_pool with every PACKET sample then it's worth considering how often it will really be retrieved.   With typical settings you will only be generating a handful of samples per second,  so retrieving the latest sample_pool every time shouldn't be too onerous(?)   At any given moment only a small number of interfaces will be "hot" and generating samples.

Neil


>> 
>> Please add "0x" here to make it obvious the value is in hex:
>> +        VLOG_WARN_RL(&rl, "invalid user cookie : %"PRIx64, upcall->userdata);
>> 
>> Please use a pointer type (e.g. uint8_t * or char *) to do pointer
>> arithmetic:
>> +    user_cookie_offset = (unsigned long) cookie -
>> +                         (unsigned long) odp_actions->data;
>> 
>> compose_controller_action() leaves some fields uninitialized.
>> 
> As other values are only required in case of sFlow. There is not need
> to initialize it.
> 
>> I don't know why add_sflow_action() initially uses
>> USER_ACTION_COOKIE_UNSPEC only to change it to
>> USER_ACTION_COOKIE_SFLOW later in fix_sflow_action().
>> 
> 
> I wanted to catch error when fix_sflow_action() is not called for
> add_sflow_action().
> But I think it is not required. I am initializing it to sFlow flag now.
> 
>> In struct action_xlate_ctx, I'd make sflow_n_outputs an 'int' or
>> 'unsigned int'.  There could be more than 255 outputs.  You can cap it
>> at 255 when you assign it to the cookie in fix_sflow_action().
>> 
>> It looks to me like the compose_actions() changes fix an existing bug
>> related to keeping track of the current VLAN correctly.  If so, we
>> should break that out as a separate commit.
>> 
> OK.
> 
>> Thanks,
>> 
>> Ben.
>> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev