[ovs-dev] [overload 3/7] ofproto: Fix effective memory leak for uninstallable flows.

[Ben Pfaff]

On Wed, Oct 06, 2010 at 11:50:33PM -0700, Justin Pettit wrote:
> Looks good.  As we discussed in person, it would be nice if there were
> a comment here describing which types of flow is being deleted in each
> case.  It's a bit confusing to the casual reader.  Also, it would be
> great if there were a descriptive comments for the rule_remove() and
> rule_uninstall() functions, since the distinction is not immediately
> obvious based on the name.

It's a good point.  Here's the revised version that I'll push out.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Thu, 30 Sep 2010 10:13:47 -0700
Subject: [PATCH] ofproto: Fix effective memory leak for uninstallable flows.

In one or two corner cases, flows cannot be installed because every packet
in the flow must be processed by userspace.  The code to expire rules was
ignoring these uninstallable rules, and thus they would never get freed,
even after they became idle.  This commit fixes the problem.
---
 ofproto/ofproto.c |   43 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ed7f5b0..5c87119 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2072,6 +2072,17 @@ rule_create_subrule(struct ofproto *ofproto, struct rule *rule,
     return subrule;
 }
 
+/* Remove 'rule' from 'ofproto' and free up the associated memory:
+ *
+ *   - If 'rule' was installed in the datapath, uninstalls it and updates
+ *     'rule''s statistics (or its super-rule's statistics, if it is a
+ *     subrule), via rule_uninstall().
+ *
+ *   - Removes 'rule' from the classifier.
+ *
+ *   - If 'rule' is a super-rule that has subrules, revalidates (and possibly
+ *     uninstalls and destroys) its subrules, via rule_destroy().
+ */
 static void
 rule_remove(struct ofproto *ofproto, struct rule *rule)
 {
@@ -2209,6 +2220,14 @@ rule_account(struct ofproto *ofproto, struct rule *rule, uint64_t extra_bytes)
     }
 }
 
+/* 'rule' must be an exact-match rule in 'p'.
+ *
+ * If 'rule' is installed in the datapath, uninstalls it and updates's
+ * statistics.  If 'rule' is a subrule, the statistics that are updated are
+ * actually its super-rule's statistics; otherwise 'rule''s own statistics are
+ * updated.
+ *
+ * If 'rule' is not installed, this function has no effect. */
 static void
 rule_uninstall(struct ofproto *p, struct rule *rule)
 {
@@ -4355,16 +4374,24 @@ rule_expire(struct cls_rule *cls_rule, void *cbdata_)
     now = time_msec();
     if (now < expire) {
         /* 'rule' has not expired according to OpenFlow rules. */
-        if (rule->installed && now >= rule->used + 5000) {
-            /* This rule is idle, so uninstall it from the datapath. */
-            if (rule->super) {
-                rule_remove(ofproto, rule);
+        if (!rule->cr.wc.wildcards) {
+            if (now >= rule->used + 5000) {
+                /* This rule is idle, so drop it to free up resources. */
+                if (rule->super) {
+                    /* It's not part of the OpenFlow flow table, so we can
+                     * delete it entirely and fold its statistics into its
+                     * super-rule. */
+                    rule_remove(ofproto, rule);
+                } else {
+                    /* It is part of the OpenFlow flow table, so we have to
+                     * keep the rule but we can at least uninstall it from the
+                     * datapath. */
+                    rule_uninstall(ofproto, rule);
+                }
             } else {
-                rule_uninstall(ofproto, rule);
+                /* Send NetFlow active timeout if appropriate. */
+                rule_active_timeout(cbdata->ofproto, rule);
             }
-        } else if (!rule->cr.wc.wildcards) {
-            /* Send NetFlow active timeout if appropriate. */
-            rule_active_timeout(cbdata->ofproto, rule);
         }
     } else {
         /* 'rule' has expired according to OpenFlow rules. */
-- 
1.7.1