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

Reply via email to