On Dec 27, 2012, at 7:23 , ext Simon Horman wrote:
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 3278e37..f1066fa 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -629,6 +629,20 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> return 0;
> }
>
> +__be16 ovs_actions_allow_l3_extraction(struct sw_flow *flow)
> +{
> + struct sw_flow_actions *acts = rcu_dereference(flow->sf_acts);
> + const struct nlattr *a;
> + int rem;
> +
> + for (a = acts->actions, rem = acts->actions_len; rem > 0;
> + a = nla_next(a, &rem))
> + if (nla_type(a) == OVS_ACTION_ATTR_POP_MPLS)
> + return *(__be16 *)nla_data(a);
> +
> + return 0;
> +}
> +
>From the use below it seems that the actions of "poorer" flows for
which the above returns non-zero will never be applied (including
the pop_mpls action itself). Also, the richer flow will need to replicate
also the pop_mpls action for it to be applied on the packet. This is
likely intended behavior, but should be documented somewhere (if
not already done so).
In the intended use case the actions list of the "poorer" flow would
have exactly one item (pop_mpls) and the
ovs_actions_allow_l3_extraction() would be fast. However, for all
other flows their whole actions list is iterated, which is unnecessary
overhead for every packet passing the datapath. Maybe there
should be a flag computed at actions set/modify time on the flow
that would be true if ovs_actions_allow_l3_extraction() will return a
non-zero ethertype? Or even better, use the ethertype as the flag, so
that the above would become something like this:
__be16 ovs_actions_allow_l3_extraction(struct sw_flow *flow)
{
return flow->l3_extraction_ethertype; /* extracted from one of our
actions */
}
> /* We limit the number of times that we pass into execute_actions()
> * to avoid blowing out the stack in the event that we have a loop. */
> #define MAX_LOOPS 5
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 747dbb6..4a15079 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -351,6 +351,7 @@ void ovs_dp_process_received_packet(struct vport *p,
> struct sk_buff *skb)
> if (!OVS_CB(skb)->flow) {
> struct sw_flow_key key;
> int key_len;
> + struct flow_table *table = rcu_dereference(dp->table);
>
> /* Extract flow from 'skb' into 'key'. */
> error = ovs_flow_extract(skb, p->port_no, &key, &key_len);
> @@ -360,8 +361,26 @@ void ovs_dp_process_received_packet(struct vport *p,
> struct sk_buff *skb)
> }
>
> /* Look up flow. */
> - flow = ovs_flow_tbl_lookup(rcu_dereference(dp->table),
> - &key, key_len);
> + flow = ovs_flow_tbl_lookup(table, &key, key_len);
> +
> + /* We have a flow? Superb.
> + * See if the actions in the flow supply information to
> + * allow more information to be included in the flow's match
> + */
> + if (likely(flow)) {
> + __be16 eth_type;
> + eth_type = ovs_actions_allow_l3_extraction(flow);
> + if (unlikely(eth_type)) {
> + error = ovs_flow_extract_l3_onwards(skb, &key,
> &key_len,
> + eth_type);
> + if (unlikely(error)) {
> + kfree_skb(skb);
> + return;
> + }
> + flow = ovs_flow_tbl_lookup(table, &key,
> key_len);
> + }
> + }
> +
> if (unlikely(!flow)) {
> struct dp_upcall_info upcall;
>
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index e44d053..96e98a4 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -196,6 +196,7 @@ struct sk_buff *ovs_vport_cmd_build_info(struct vport *,
> u32 portid, u32 seq,
> u8 cmd);
>
> int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb);
> +__be16 ovs_actions_allow_l3_extraction(struct sw_flow *flow);
>
> void skb_cb_set_mpls_bos(struct sk_buff *skb);
> unsigned char *skb_cb_mpls_bos(const struct sk_buff *skb);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
