[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