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

[Pravin Shelar]

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