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.
I didn't really read the kernel changes.
In format_odp_sample_action(), please just go ahead and use "double"
for 'percentage' and 100.0 instead of 100.0f. "float" is only for
situations when you've got an actual shortage of memory or time (also,
UINT32_MAX can be exactly represented as a double but not as a float).
Some of the indentation in userspace looks funny, e.g. this:
if (cookie.type == USER_ACTION_COOKIE_CONTROLLER) {
ds_put_format(ds, "userspace(controller,data=%"PRIu32")",
cookie.data);
} else if (cookie.type == USER_ACTION_COOKIE_SFLOW) {
ds_put_format(ds, "userspace(sFlow,n_output=%"PRIu8","
"vid=%"PRIu16",pcp=%"PRIu8",ifindex=%"PRIu32")",
cookie.n_output, vlan_tci_to_vid(cookie.vlan_tci),
vlan_tci_to_pcp(cookie.vlan_tci), cookie.data);
} else {
ds_put_format(ds, "userspace(Unknown,data=%"PRIx64")",
nl_attr_get_u64(a));
}
should be indented as:
if (cookie.type == USER_ACTION_COOKIE_CONTROLLER) {
ds_put_format(ds, "userspace(controller,data=%"PRIu32")",
cookie.data);
} else if (cookie.type == USER_ACTION_COOKIE_SFLOW) {
ds_put_format(ds, "userspace(sFlow,n_output=%"PRIu8","
"vid=%"PRIu16",pcp=%"PRIu8",ifindex=%"PRIu32")",
cookie.n_output, vlan_tci_to_vid(cookie.vlan_tci),
vlan_tci_to_pcp(cookie.vlan_tci), cookie.data);
} else {
ds_put_format(ds, "userspace(Unknown,data=%"PRIx64")",
nl_attr_get_u64(a));
}
and I see other funny indentation elsewhere too.
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?
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.
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().
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.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
