On Wed, Oct 26, 2011 at 11:32 AM, Pravin B Shelar <pshe...@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)?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
