[ovs-dev] [PATCH v2] datapath: Refactor actions in terms of match fields.

Pravin Shelar pshelar at nicira.com
Wed Oct 19 23:03:41 UTC 2011


On Wed, Oct 19, 2011 at 2:07 PM, Ben Pfaff <blp at nicira.com> wrote:
> I didn't really read the kernel code.
>
> <linux/openvswitch.h>
> ---------------------
>
> I think that the intention is that OVS_ACTION_ATTR_{SET,PUSH} contain
> exactly one nested attribute, but the comment on it doesn't say that.
> It's common to support a bunch of nested attributes, and so I could
> see someone trying to set several keys in a single
> OVS_ACTION_ATTR_SET, so it's probably a good idea to mention that only
> a single field can be set with a single OVS_ACTION_ATTR_{SET,PUSH}.
>
> I think that the new comment should explain each possibility
> explicitly, like the comment for enum ovs_flow_attr does.  I believe
> that currently it is really talking about OVS_ACTION_ATTR_SET and
> OVS_ACTION_ATTR_PUSH, but it doesn't really say so.
>
> The new comment doesn't have the right form for a kerneldoc comment.
> It should start with "enum ovs_action_attr - " followed by a brief
> description that fits on the first line.  And I think that it should
> be just above the enum declaration, so that you should delete the /*
> Action types. */ comment and the blank line.
>
> userspace
> ---------
>
> In dp_netdev_set_dl(), I don't see a benefit to doing the
> comparisons.  I think that we can just copy in the new Ethernet source
> and destination directly.
>
> This line is wrong (s/udp_dst/udp_src/):
> +    if (uh->udp_dst != udp_key->udp_src) {
>
> In execute_set_action(), please assign the nl_attr_type value to an
> "enum ovs_key_attr" and then switch on that, adding a case for each
> existing OVS_KEY_ATTR_*.  That way, we'll get a warning when we add a
> new OVS_KEY_ATTR_* type, to remind us to implement it.
>
> Are OVS_ACTION_ATTR_PUSH and OVS_ACTION_ATTR_POP so simple because we
> only support pushing and popping VLAN tags?  Then I'd like an
> assertion in there to verify that the inner attribute type is
> OVS_KEY_ATTR_8021Q.
>
> {} are needed here around the statements subordinate to "if":
> +    case OVS_ACTION_ATTR_POP:
> +        if (nl_attr_get_u16(a) == OVS_KEY_ATTR_8021Q)
> +            ds_put_cstr(ds, "pop(vlan)");
> +        else
> +            ds_put_format(ds, "pop(key=%"PRIu16")", nl_attr_get_u16(a));
>
> In facet_account(), q_key can be declared inside the "if" statement
> instead of at a higher level.
>
> Please don't give a function in ofproto-dpif.c a name beginning with
> nl_, since that prefix belongs to lib/netlink.c.  I think you can
> rename nl_msg_put_nested_key() to put_nested_key().  Or it could
> reasonably go in odp-util.c under some appropriate name.  In fact, it
> seems appropriate to me to put the bulk of the new code in the
> commit_*() functions into odp-util.c; ofproto-dpif.c is already too
> big and this seems like a clean separation.
>

ok, next patch (related to SET_PRIORITY) that I am working on will
make all commit_odp_ functions independent of ctx. Then I will move
them to odp-utils.

I will fix patch according to rest of comments.

> I think that the new version of commit_vlan_action() can be slightly
> simplified to:
>
> static void
> commit_vlan_action(struct action_xlate_ctx *ctx, ovs_be16 new_tci)
> {
>    struct flow *base = &ctx->base_flow;
>
>    if (base->vlan_tci == new_tci) {
>        return;
>    }
>
>    if (base->vlan_tci & htons(VLAN_CFI)) {
>        nl_msg_put_u16(ctx->odp_actions, OVS_ACTION_ATTR_POP,
>                       OVS_KEY_ATTR_8021Q);
>    }
>    if (new_tci & htons(VLAN_CFI)) {
>        struct ovs_key_8021q q_key;
>
>        q_key.q_tpid = htons(ETH_TYPE_VLAN);
>        q_key.q_tci = new_tci & ~htons(VLAN_CFI);
>
>        nl_msg_put_nested_key(ctx->odp_actions, OVS_ACTION_ATTR_PUSH,
>                              OVS_KEY_ATTR_8021Q, &q_key, sizeof(q_key));
>    }
>    base->vlan_tci = new_tci;
> }
>
> ipv4_key and ipv6_key end with some padding, so it might be a good
> idea to memset() them to 0 before initializing.
>
> It looks like commit_set_nw_action() and commit_set_port_action()
> forbid setting IPv4 addresses and TCP/UDP ports to 0.  Maybe you want
> to check 'base' instead of 'flow' for nonzero?
>
>
> Thanks,
>
> Ben.
>



More information about the dev mailing list