[ovs-dev] [PATCH 03/18] datapath: re-lookup a flow if actions allow a more fine grained match

[Simon Horman]

On Thu, Dec 27, 2012 at 07:25:29PM +0200, Jarno Rajahalme wrote:
> 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 */
> }

Thanks. You are correct and that seems like it would be a useful
optimisation. I'll see about adding it.