[ovs-dev] [PATCH 1/6] ofp-actions: Add the NXAST_SAMPLE vendor action
Romain Lenglet
rlenglet at vmware.com
Thu Apr 18 02:00:17 UTC 2013
Hi Ben,
Thanks a lot for your detailed review!
Comments inline below.
On Apr 16, 2013, at 2:30 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Mar 25, 2013 at 06:33:02PM -0700, Romain Lenglet wrote:
>> Define NXAST_SAMPLE OpenFlow vendor action and the corresponding
>> OFPACT_SAMPLE OVS action, to do per-flow packet sampling.
>>
>> Signed-off-by: Romain Lenglet <rlenglet at vmware.com>
>
> Thanks for the revised series! Sorry it took a long time to review.
>
> This time I reviewed the series after squashing everything together.
> It was easier than I expected.
>
> There is still one TODO that looks wrong to me after squashing:
> /* TODO: Send a sample packet to each sFlow collector. */
> in handle_flow_sample_upcall().
Removed.
> I also wonder about this one in ofproto_set_ipfix():
> /* TODO: Check the options. */
Removed.
> Somewhere in this series I think we should mention in the FAQ that for
> IPFIX to work we need the out-of-kernel kernel module from OVS version
> 1.x or later, or the in-tree kernel module from Linux version 3.y or
> later. I don't know off-hand what 'x' or 'y' should be (let me know
> if you can't figure them out), but it's whatever version added the
> variable-length userdata for upcalls.
Done in the "ofproto: Translate NXAST_SAMPLE actions into SAMPLE "flow_sample" dp actions" change, which adds the first non-64-bit cookie structure.
Your patch was not included in 1.9.x or upstream, so I documented that this requires the module from OVS 1.10.90 or later.
> nicira-ext.h
> ------------
>
> The comment on 'len' here should state the length is 24:
> struct nx_action_sample {
> ovs_be16 type; /* OFPAT_VENDOR. */
> ovs_be16 len; /* Length is 16. */
Done.
> odp-util.c
> ----------
>
> In format_odp_userspace_action(), some of the indentation is a little
> off in the 'if' statements. This:
> } else if (userdata_len == sizeof cookie.slow_path
> && cookie.type == USER_ACTION_COOKIE_SLOW_PATH) {
> should be:
> } else if (userdata_len == sizeof cookie.slow_path
> && cookie.type == USER_ACTION_COOKIE_SLOW_PATH) {
> and there are extra blank lines e.g. two of them here:
> } else if (userdata_len == sizeof cookie.ipfix
> && cookie.type == USER_ACTION_COOKIE_IPFIX) {
>
> ds_put_format(ds, ",ipfix");
>
> } else {
Done.
> parse_odp_action() has some parameter validation that we generally
> don't do:
> if (probability == 0) {
> return -INVAR;
> }
> We generally don't validate parameters at this level because that
> gives us a better opportunity to test implementations (e.g. the kernel
> and userspace datapaths) against bad input. (But we don't do much of
> that, so if you feel strongly that we should reject this bad
> parameter, go ahead and leave the check.)
Agreed. Removed.
> ofp-actions.c
> -------------
>
> Indentation is off here:
> static void
> ofpact_sample_to_nxast(const struct ofpact_sample *os,
> struct ofpbuf *out)
Done.
> ofp-parse.c
> -----------
>
> There's an doubled blank line here in parse_named_action():
> case OFPUTIL_NXAST_STACK_POP:
> nxm_parse_stack_action(ofpact_put_STACK_POP(ofpacts), arg);
> break;
>
>
> case OFPUTIL_NXAST_SAMPLE:
> parse_sample(ofpacts, arg);
> break;
Removed.
> ofproto-dpif-ipfix.c
> --------------------
>
> <config.h> should be the first include file. I don't think you need
> <netinet/in.h> at all; byte-order.h includes it and it is reasonable
> to rely on that.
Removed.
> The return value convention for dpif_ipfix_exporter_set_options() and
> dpif_ipfix_bridge_exporter_set_options() are unusual by OVS standards.
> Usually, an OVS function that returns only success or failure has a
> "bool" return type.
Done: I made those true on success, false on failure.
> This comment is puzzling. size_t is at least 32 bits on the systems
> we target.
> /* Hash into 16 bits since that's the standard size for size_t. */
> #define COLLECTOR_SET_ID_HASH(ID) (((ID) & 0xffff) ^ ((ID) >> 16))
> Also, why not use hash_int() instead of this inlined hash? And why a
> macro instead of a function?
Replaced with hash_int(). I didn't know about that function, thanks.
> dpif_ipfix_set_options() has two assertions that are commented out,
> but I don't know why. Are the assertions not actually correct?
They are correct, and I have put them back.
I wasn't sure about the policy regarding asserts, since there aren't many in the code.
> dpif_ipfix_set_options() stops adding flow exporters if adding one
> fails, and does not add any more flow exporters or remove flow
> exporters that should be removed. Shouldn't it do both?
OK, I've changed to just:
- log at WARN level;
- continue configuring the remaining exporters.
> This code in dpif_ipfix_set_options() looks wrong, because the code
> after the "break" is unreachable. I think that the options++ should
> be outside the inner block:
>
> for (i = 0; i < n_flow_exporters_options; i++) {
> if (node->exporter.options->collector_set_id
> == options->collector_set_id) {
> break;
> options++;
> }
> }
Fixed.
Good catch, thanks!
> dpif_ipfix_clear() is non-static and has a prototype in
> ofproto-dpif-ipfix.h, but I don't think that any code outside
> ofproto-dpif-ipfix.c can safely use it because it destroys the ofpbuf
> member 'msg' and later uses of ofproto-dpif-ipfix.c functions will use
> that member.
Made this function static and removed it from ofproto-dpif-ipfix.h.
Removed the 'msg' un/initialization, as I switched to a temp buffer, cf. below.
> Regarding 'msg', actually, it looks like this is only used temporarily
> within ipfix_send_template_msg() and ipfix_send_data_msg(). For that
> kind of use, we would usually allocate the buffer on-stack, e.g.:
> uint64_t msg_stub[DIV_ROUND_UP(1500, 8)];
> struct ofpbuf msg;
>
> ofpbuf_use_stub(&msg, msg_stub, sizeof msg_stub);
> ...
> ofpbuf_uninit(&msg);
I've switched to using that. Thanks for the advice.
> You could use ofpbuf_use_stack() instead, and avoid the need for
> ofpbuf_uninit(), if you are certain that the buffer contents will
> never be longer than the allocated space.
>
> In ipfix_send_data_msg(), many of the local variables could be
> declared in inner blocks, which is preferred style in OVS.
Done.
> In the definition of dpif_ipfix_flow_sample(), the return type
> ("void") should be on a line by itself.
Done.
> ofproto-dpif-ipfix.h
> ---------------------
>
> This header declares of "struct ofproto_ipfix_options" but there is no
> definition of this struct anywhere in the tree.
Removed.
> utilities/ovs-ofctl.8.in
> ------------------------
>
> The action description should now mention OVS 1.10.90 instead of
> 1.9.90 (sorry about that).
Fixed.
> utilities/ovs-vsctl.c
> ---------------------
>
> It seems that we should change del_bridge() to delete any
> FlowSampleCollectorSet records that reference the bridge. Otherwise
> the user will get a cryptic error.
Done.
> vswitchd/bridge.c
> -----------------
>
> In bridge_configure_ipfix(), some of the local variable names are
> really long and make the code harder to read. Maybe, for example,
> bridge_exporter_options could be shortened to, say, be_opts, and
> bridge_exporter_cfg to be_cfg?
Good idea. Done.
> bridge_configure_ipfix() dereferences
> bridge_exporter_cfg->obs_domain_id and
> bridge_exporter_cfg->obs_point_id without checking that they are
> nonnull, but they will be nonnull if the user does not configure them
> in the database.
Fixed.
> vswitchd.ovsschema
> ------------------
>
> I think that Flow_Sample_Collector_Set would better fit the naming
> convention we have so far.
Done.
> vswitch.xml
> -----------
>
> There is no explanation of FlowSampleCollectorSet's bridge column.
Added.
> Thanks,
>
> Ben.
More information about the dev
mailing list