Ben,
Thanks for fixing that up, looks good :-)
Jarno
On Mar 8, 2013, at 18:52 , ext Ben Pfaff wrote:
> On Fri, Mar 08, 2013 at 03:50:12PM +0200, Jarno Rajahalme wrote:
>> Remove recursion from GOTO_TABLE processing in do_xlate_actions().
>> This allows packet processing pipelines built with goto table be
>> longer and not interact with each other via the resubmit recursion limit.
>>
>> Signed-off-by: Jarno Rajahalme <jarno.rajaha...@nsn.com>
>
> Thank you, this is a nice refinement.
>
> This looks very close to correct to me. However, I don't think that the
> handling of the 'evictable' flags is quite right. The basic rule for
> 'evictable' is that we need to make sure that it is set for any rule
> that we're currently "using" in some way (one example is iterating over
> its actions) to tell "learn" actions not to delete that rule. As I read
> your patch, it didn't do this for rules reached via "goto_table".
> (Probably because the purpose of 'evictable' really isn't obvious.)
>
> I applied the following incremental to fix that. It seems correct, so
> I'll apply it to master soon, but please check my work.
>
> Thanks,
>
> Ben.
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6628aa6..d911080 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6305,7 +6305,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> {
> bool was_evictable = true;
> const struct ofpact *a;
> - struct rule_dpif *orig_rule = ctx->rule;
>
> if (ctx->rule) {
> /* Don't let the rule we're working on get evicted underneath us. */
> @@ -6509,7 +6508,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> rule = ctx_rule_hooks(ctx, rule, true);
>
> if (rule) {
> + if (ctx->rule) {
> + ctx->rule->up.evictable = was_evictable;
> + }
> ctx->rule = rule;
> + was_evictable = rule->up.evictable;
> + rule->up.evictable = false;
>
> /* Tail recursion removal. */
> ofpacts = rule->up.ofpacts;
> @@ -6522,7 +6526,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> }
>
> out:
> - ctx->rule = orig_rule;
> if (ctx->rule) {
> ctx->rule->up.evictable = was_evictable;
> }
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
