[ovs-dev] [PATCH 11/12] datapath: Replace "struct odp_action" by Netlink attributes.

Jesse Gross jesse at nicira.com
Wed Dec 8 03:48:08 UTC 2010


On Tue, Dec 7, 2010 at 11:00 AM, Ben Pfaff <blp at nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index c8b5088..3a21236 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -516,7 +516,7 @@ void dp_process_received_packet(struct vport *p, struct sk_buff *skb)
> +        const struct nlattr *a;
> +        int rem;
> +
> +        nla_for_each_attr (a, actions, actions_len, rem) {

There's an extra space before the opening parenthesis.

> +                static const int action_lens[ODPAT_MAX + 1] = {
> +                        [ODPAT_OUTPUT] = 4,
> +                        [ODPAT_CONTROLLER] = 4,
> +                        [ODPAT_SET_DL_TCI] = 2,
> +                        [ODPAT_STRIP_VLAN] = 0,
> +                        [ODPAT_SET_DL_SRC] = ETH_ALEN,
> +                        [ODPAT_SET_DL_DST] = ETH_ALEN,
> +                        [ODPAT_SET_NW_SRC] = 4,
> +                        [ODPAT_SET_NW_DST] = 4,
> +                        [ODPAT_SET_NW_TOS] = 1,
> +                        [ODPAT_SET_TP_SRC] = 2,
> +                        [ODPAT_SET_TP_DST] = 2,
> +                        [ODPAT_SET_TUNNEL] = 8,

As of this patch, the argument for ODPAT_SET_TUNNEL is only 4 bytes.

> @@ -709,16 +736,14 @@ static struct sw_flow_actions *get_actions(const struct odp_flow *flow)
>        struct sw_flow_actions *actions;
>        int error;
>
> -       actions = flow_actions_alloc(flow->n_actions);
> +       actions = flow_actions_alloc(flow->actions_len);
>        error = PTR_ERR(actions);
>        if (IS_ERR(actions))
>                goto error;
>
> -       error = -EFAULT;
> -       if (copy_from_user(actions->actions, flow->actions,
> -                          flow->n_actions * sizeof(union odp_action)))
> +       if (copy_from_user(actions->actions, flow->actions, flow->actions_len))
>                goto error_free_actions;

This won't return the correct error if copy_from_user() fails.

> diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
> index 8e07b8b..8658d59 100644
> --- a/include/openvswitch/datapath-protocol.h
> +++ b/include/openvswitch/datapath-protocol.h
> @@ -240,8 +241,8 @@ struct odp_flow_key {
>  struct odp_flow {
>     struct odp_flow_stats stats;
>     struct odp_flow_key key;
> -    union odp_action *actions;
> -    uint32_t n_actions;
> +    struct nlattr *actions;
> +    unsigned int actions_len;

I think that uint32_t is more appropriate for interface definitions
than an int (here and in the other structs in this file).

> +enum odp_action_type {
> +    ODPAT_UNSPEC,
> +    ODPAT_OUTPUT = 1,            /* Output to switch port. */
> +    ODPAT_CONTROLLER = 2,        /* Send copy to controller. */
> +    ODPAT_SET_DL_TCI = 3,        /* Set the 802.1q TCI value. */
> +    ODPAT_STRIP_VLAN = 4,        /* Strip the 802.1q header. */
> +    ODPAT_SET_DL_SRC = 5,        /* Ethernet source address. */
> +    ODPAT_SET_DL_DST = 6,        /* Ethernet destination address. */
> +    ODPAT_SET_NW_SRC = 7,        /* IP source address. */
> +    ODPAT_SET_NW_DST = 8,        /* IP destination address. */
> +    ODPAT_SET_NW_TOS = 9,        /* IP ToS/DSCP field (6 bits). */
> +    ODPAT_SET_TP_SRC = 10,        /* TCP/UDP source port. */
> +    ODPAT_SET_TP_DST = 11,       /* TCP/UDP destination port. */
> +    ODPAT_SET_TUNNEL = 12,       /* Set the encapsulating tunnel ID. */
> +    ODPAT_SET_PRIORITY = 13,     /* Set skb->priority. */
> +    ODPAT_POP_PRIORITY = 14,     /* Restore original skb->priority. */
> +    ODPAT_DROP_SPOOFED_ARP = 15, /* Drop ARPs with spoofed source MAC. */
> +
> +    __ODPAT_MAX,
> +    ODPAT_MAX = __ODPAT_MAX - 1
>  };

The Linux header files don't explicitly assign values to the members
of the Netlink enums.  I actually would generally prefer to be
explicit about it but it might be more important to be consistent.

Also, the _MAX values are usually done as a #define outside of the
enum.  I agree with that one.

> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 6b4f5fa..8089cd3 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> +int
> +odp_action_len(uint16_t type)
> +{
> +    if (type > ODPAT_MAX) {
> +        return -1;
> +    }
> +
> +    switch ((enum odp_action_type) type) {
> +    case ODPAT_OUTPUT: return 4;
> +    case ODPAT_CONTROLLER: return 4;
> +    case ODPAT_SET_DL_TCI: return 2;
> +    case ODPAT_STRIP_VLAN: return 0;
> +    case ODPAT_SET_DL_SRC: return ETH_ADDR_LEN;
> +    case ODPAT_SET_DL_DST: return ETH_ADDR_LEN;
> +    case ODPAT_SET_NW_SRC: return 4;
> +    case ODPAT_SET_NW_DST: return 4;
> +    case ODPAT_SET_NW_TOS: return 1;
> +    case ODPAT_SET_TP_SRC: return 2;
> +    case ODPAT_SET_TP_DST: return 2;
> +    case ODPAT_SET_TUNNEL: return 8;

ODPAT_SET_TUNNEL should be 4 here as well.

> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 7a93b10..88b7124 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
>  struct action_xlate_ctx {
>     /* Input. */
>     struct flow flow;           /* Flow to which these actions correspond. */
> @@ -2582,7 +2575,9 @@ struct action_xlate_ctx {
>                                   * without a packet to refer to. */
>
>     /* Output. */
> -    struct odp_actions *out;    /* Datapath actions. */
> +    struct ofpbuf *odp_actions; /* Datapath actions. */
> +    int last_pop_priority;      /* Offset in 'odp_actions' just past most
> +                                 * recently * added ODPAT_SET_PRIORITY. */

There's an extra asterisk in the comment for last_pop_priority.

> @@ -2835,7 +2837,6 @@ xlate_nicira_action(struct action_xlate_ctx *ctx,
>     const struct nx_action_resubmit *nar;
>     const struct nx_action_set_tunnel *nast;
>     const struct nx_action_set_queue *nasq;
> -    union odp_action *oa;
>     int subtype = ntohs(nah->subtype);
>
>     assert(nah->vendor == htonl(NX_VENDOR_ID));
> @@ -2847,13 +2848,12 @@ xlate_nicira_action(struct action_xlate_ctx *ctx,
>
>     case NXAST_SET_TUNNEL:
>         nast = (const struct nx_action_set_tunnel *) nah;
> -        oa = odp_actions_add(ctx->out, ODPAT_SET_TUNNEL);
> -        ctx->flow.tun_id = oa->tunnel.tun_id = nast->tun_id;
> +        nl_msg_put_be32(ctx->odp_actions, ODPAT_SET_TUNNEL, nast->tun_id);

This no longer updates the flow in ctx.  Is there a reason for that?

> @@ -4398,12 +4395,14 @@ handle_odp_miss_msg(struct ofproto *p, struct ofpbuf *packet)
>     /* Check with in-band control to see if this packet should be sent
>      * to the local port regardless of the flow table. */
>     if (in_band_msg_in_hook(p->in_band, &flow, &payload)) {
> -        union odp_action action;
> +        struct ofpbuf odp_actions;
> +        uint64_t buf[1];

Why is this an array of size 1?

> +
> +        ofpbuf_use_stack(&odp_actions, buf, sizeof buf);

What is this function?  Neither I nor the compiler can find it.

> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
> index bed50fa..8f2a2bc 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -478,11 +478,11 @@ do_dump_flows(int argc OVS_UNUSED, char *argv[])
>     ds_init(&ds);
>     for (i = 0; i < n_flows; i++) {
>         struct odp_flow *f = &flows[i];
> -        enum { MAX_ACTIONS = 4096 / sizeof(union odp_action) };
> -        union odp_action actions[MAX_ACTIONS];
> +        enum { MAX_ACTIONS = 4096 }; /* An arbitrary but large number. */
> +        struct nlattr actions[MAX_ACTIONS];
>
>         f->actions = actions;
> -        f->n_actions = MAX_ACTIONS;
> +        f->actions_len = sizeof actions;
>         if (!dpif_flow_get(dpif, f)) {

The comments in dpif.c above dpif_flow_get() and
dpif_flow_get_multiple() still refer to n_actions instead of
actions_len.




More information about the dev mailing list