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

Ben Pfaff blp at nicira.com
Thu Dec 9 20:52:04 UTC 2010


On Tue, Dec 07, 2010 at 07:48:08PM -0800, Jesse Gross wrote:
> 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.

Hmm, I see now that that is the kernel style.  Fixed.

> > +                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.

Fixed, thanks.

> > @@ -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.

Oops, thanks for pointing that out.  Fixed.

> > 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).

OK, done.

> > +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.

OK, changed.

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

OK, fine, done.

> > 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.

Thanks, fixed.

> > 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.

Thanks, fixed.

> > @@ -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?

Oops, that's a bug.  Fixed.

> > @@ -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.

The above were an experiment that I backed out, but I goofed and only
did the back-out in the following commit.

Anyway, here's what should have been here in this commit, which I've
fixed up now:

    /* 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)) {
        struct ofpbuf odp_actions;

        ofpbuf_init(&odp_actions, 32);
        nl_msg_put_u32(&odp_actions, ODPAT_OUTPUT, ODPP_LOCAL);
        dpif_execute(p->dpif, odp_actions.data, odp_actions.size, &payload);
        ofpbuf_uninit(&odp_actions);
    }

> > 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.

Thanks, I fixed these up.

I'll re-send this series in a few minutes.




More information about the dev mailing list