I only skimmed the kernel code but I noticed the following:
> @@ -428,6 +432,7 @@ enum ovs_action_type {
> OVS_ACTION_ATTR_SET_TUNNEL, /* Set the encapsulating tunnel ID. */
> OVS_ACTION_ATTR_SET_PRIORITY, /* Set skb->priority. */
> OVS_ACTION_ATTR_POP_PRIORITY, /* Restore original skb->priority. */
> + ODP_ACTION_ATTR_SAMPLE, /* SAMPLE action */
The above comment doesn't help much; the reader can already see that
without the comment.
ofproto.h is a poor place for "struct user_data", since it is only
used by ofproto-dpif and odp-util. I would move it to odp-util.h. It
is also poorly named; it could mean anything.
The use of bit-fields in struct user_data is not customary in Open
vSwitch. We already have lots of fields that include a VLAN TCI field
as a 16-bit ovs_be16, so I'd use that format for consistency. There
are functions vlan_tci_to_vid() and vlan_tci_to_pcp() in lib/packets.h
if you need to extract subfields.
In struct user_data, please don't use an anonymous union; some
compilers don't support that.
In odp_action_len(), why is 24 the length of ODP_ACTION_ATTR_SAMPLE?
I think that "sample" actions are variable in length (can't you put
whatever you want in the embedded actions?). Probably we need to not
fix the length of ODP_ACTION_ATTR_SAMPLE.
In format_odp_sample_action(), dividing integer 100 by an integer
gives really bad precision. This function can only print 100, 50, 33,
25, 20, 16, 14, and 12 down to 0 as percentages (only about 20% of the
integers from 0 to 100). Barring a reason why not, I'd just use
floating point:
uint32_t probability;
...
probability = nl_attr_get_u32(a[OVS_SAMPLE_ATTR_PROBABILITY]);
ds_put_format(ds, "(sample=%.1f"%%", 1.0 * UINT32_MAX / probability);
I'm surprised that OVS_SAMPLE_ATTR_ACTIONS is optional. I don't think
that a sample action is useful without any embedded actions.
In format_odp_sample_action(), "ifindix" => "ifindex".
In format_odp_action(), please put {} around the conditional
statements in "if" blocks, in userspace.
In the declaration of format_odp_sample_action(), please
please put function names on a separate line in userspace, e.g.:
static void
format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
I see at least one other instance.
I don't yet understand why commit_odp_actions() and compose_actions()
add the sFlow action and then fix it up. Why not add it from
xlate_actions() just before calling do_xlate_actions(), and fix up
just after calling do_xlate_actions()?
There's a number of repetitions of roughly:
nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_OUTPUT, odp_port);
ctx->sflow_odp_port = odp_port;
ctx->sflow_n_outputs++;
now, so would it make sense to add a helper function for that?
I suspect that ports added by mirroring shouldn't count as sflow
output ports. If it's tricky to deal with that, I don't think that we
need to worry about it yet; it's not a regression.
fix_sflow_action() could be much simpler--it wouldn't need to do any
searching--if compose_sflow_action() saved the offset of the user_data
inside the odp_actions.
In set_sflow(), the call to dpif_sflow_destroy(ds) could go inside the
"if (ds)" block.
In ofproto-dpif-sflow.h, please write
void dpif_sflow_received(struct dpif_sflow *ds,
struct ofpbuf *packet,
const struct flow *flow,
const struct user_data *data);
int
dpif_sflow_odp_port_to_ifindex(const struct dpif_sflow *ds,
uint16_t odp_port);
as
void dpif_sflow_received(struct dpif_sflow *,
struct ofpbuf *packet,
const struct flow *,
const struct user_data *);
int dpif_sflow_odp_port_to_ifindex(const struct dpif_sflow *,
uint16_t odp_port);
In other words, put the type and the function name on a single line,
and omit parameter names when they don't aid understanding in some
way.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
