[ovs-dev] [PATCH] datapath: Convert kernel priority actions into match/set.

Jesse Gross jesse at nicira.com
Thu Oct 27 03:17:21 UTC 2011


On Wed, Oct 26, 2011 at 11:32 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index be90d54..4809d4b 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -770,7 +764,8 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>        if (err)
>                goto err_flow_put;
>
> -       err = flow_metadata_from_nlattrs(&flow->key.eth.in_port,
> +       err = flow_metadata_from_nlattrs(&flow->key.priority,
> +                                        &flow->key.eth.in_port,
>                                         &flow->key.eth.tun_id,
>                                         a[OVS_PACKET_ATTR_KEY]);

I think we also want to assign the priority to the packet as well so
it really does act like it came in with the specified metadata.

> diff --git a/datapath/flow.c b/datapath/flow.c
> index b6023a0..98b6aec 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -915,11 +918,17 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
>
>  #define TRANSITION(PREV_TYPE, TYPE) (((PREV_TYPE) << 16) | (TYPE))
>                switch (TRANSITION(prev_type, type)) {
> +               case TRANSITION(OVS_KEY_ATTR_UNSPEC, OVS_KEY_ATTR_PRIORITY):

Can you update the comment at the beginning of the function with the
new state machine?

> +int flow_metadata_from_nlattrs(u32 *priority, u16 *in_port, __be64 *tun_id,
>                               const struct nlattr *attr)
>  {
>        const struct nlattr *nla;
> @@ -1166,11 +1177,17 @@ int flow_metadata_from_nlattrs(u16 *in_port, __be64 *tun_id,
>                        return -EINVAL;
>
>                switch (TRANSITION(prev_type, type)) {
> +               case TRANSITION(OVS_KEY_ATTR_UNSPEC, OVS_KEY_ATTR_PRIORITY):
> +                       *priority = nla_get_u32(nla);
> +                       break;

You should initialize *priority to 0 at beginning of this function.

> diff --git a/datapath/flow.h b/datapath/flow.h
> index 484ea62..e7a8c77 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -35,6 +35,7 @@ struct sw_flow_actions {
>  #define OVS_FRAG_TYPE_MASK INET_ECN_MASK
>
>  struct sw_flow_key {
> +       u32     priority;               /* Packet QoS priority. */

By putting the priority here you're introducing an unnecessary hole on
64-bit platforms because tun_id wants to be 8 byte aligned.  I think
this new field warrants some rearranging.  I would pull tun_id and
in_port out of the eth struct and create a new 'phy' struct at the
beginning with tun_id, priority, and in_port (in that order).  This
leaves a 2 byte hole at the end but I now see that we already have one
at the end of eth currently.  It is possible to completely eliminate
it but we're going to need to add a few small new members in the near
future that will reintroduce it so we might as well do it the clean
way.

> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 83d56d6..815c321 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -318,8 +318,7 @@ struct dpif_class {
>     int (*recv_set_mask)(struct dpif *dpif, int listen_mask);
>
>     /* Translates OpenFlow queue ID 'queue_id' (in host byte order) into a
> -     * priority value for use in the OVS_ACTION_ATTR_SET_PRIORITY action in
> -     * '*priority'. */
> +     * priority value for use in the ovs-set-priority action in '*priority'. */

There isn't a ovs-set-priority action, so maybe we should just make it
generic and say "when setting the priority" (this also shows up in
dpif.c).

> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 33672c8..c83ec55 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -257,6 +249,7 @@ odp_flow_key_attr_len(uint16_t type)
>     }
>
>     switch ((enum ovs_key_attr) type) {
> +    case OVS_KEY_ATTR_PRIORITY: return sizeof(uint32_t);

Other places in this switch we just have the value for simple types.

> @@ -338,6 +331,10 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
>     }
>
>     switch (nl_attr_type(a)) {
> +    case OVS_KEY_ATTR_PRIORITY:
> +        ds_put_format(ds, "QoS(priority=%"PRIu32")", nl_attr_get_u32(a));

I think the consistent formatting here would be "priority(%"PRIu32")".

> @@ -528,6 +525,16 @@ parse_odp_key_attr(const char *s, struct ofpbuf *key)
>      * type larger than 64 bits. */
>
>     {
> +        unsigned long long int priority;
> +        int n = -1;
> +
> +        if (sscanf(s, "QoS(priority=%lli)%n", &priority, &n) > 0 && n > 0) {

And then this would need to be updated as well.

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index b53452d..7ef87f1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c

It looks to me like priority doesn't actually make it into struct
flow.  In handle_miss_upcall(), the priority is put into flow by
odp_flow_key_to_flow() but then flow_extract() memsets the flow to
zero.

> +commit_set_priority_action(const struct flow *flow, struct flow *base,
> +                           struct ofpbuf *odp_actions)
[...]
> +    commit_action__(odp_actions, OVS_ACTION_ATTR_SET,
> +                    OVS_KEY_ATTR_PRIORITY, &base->priority,
> +                    sizeof(uint32_t));

I think doing something like sizeof(base->priority) might be more useful.

> @@ -3978,10 +3979,8 @@ xlate_enqueue_action(struct action_xlate_ctx *ctx,
>     odp_port = ofp_port_to_odp_port(ofp_port);
>
>     /* Add datapath actions. */
> -    ctx_priority = ctx->priority;
> -    ctx->priority = priority;
> +    ctx->flow.priority = priority;
>     add_output_action(ctx, odp_port);
> -    ctx->priority = ctx_priority;

Don't you need to reset the priority after the output action?

Ben, what do you think of this approach of zeroing out the priority
(or any other comments)?



More information about the dev mailing list