[ovs-dev] [PATCH 3/7] ofproto: Optimize the case of a repeated learn action execution.

[Jarno Rajahalme]

When the target flow exists already as intended, only the 'modified'
time needs to be updated.  Allow modification without taking the
'ofproto_mutex' by always using the 'rule->mutex' when accessing the
'modified' time.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 ofproto/ofproto-provider.h |   18 +++++++++++++-----
 ofproto/ofproto.c          |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 19d1551..77ca498 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -339,7 +339,9 @@ struct rule {
 
     /* Protects members marked OVS_GUARDED.
      * Readers only need to hold this mutex.
-     * Writers must hold both this mutex AND ofproto_mutex. */
+     * Writers must hold both this mutex AND ofproto_mutex.
+     * By implication writers can read *without* taking this mutex while they
+     * hold ofproto_mutex. */
     struct ovs_mutex mutex OVS_ACQ_AFTER(ofproto_mutex);
 
     /* Number of references.
@@ -356,10 +358,6 @@ struct rule {
     ovs_be64 flow_cookie OVS_GUARDED;
     struct hindex_node cookie_node OVS_GUARDED_BY(ofproto_mutex);
 
-    /* Times. */
-    long long int created OVS_GUARDED; /* Creation time. */
-    long long int modified OVS_GUARDED; /* Time of last modification. */
-    long long int used OVS_GUARDED; /* Last use; time created if never used. */
     enum ofputil_flow_mod_flags flags OVS_GUARDED;
 
     /* Timeouts. */
@@ -393,6 +391,16 @@ struct rule {
     /* Optimisation for flow expiry.  In ofproto's 'expirable' list if this
      * rule is expirable, otherwise empty. */
     struct list expirable OVS_GUARDED_BY(ofproto_mutex);
+
+    /* Times.  Last so that they are more likely close to the stats managed
+     * by the provider. */
+    long long int created OVS_GUARDED; /* Creation time. */
+
+    /* Must hold 'mutex' for both read/write, 'ofproto_mutex' not needed. */
+    long long int modified OVS_GUARDED; /* Time of last modification. */
+
+    /* XXX: Currently updated by provider without protection. */
+    long long int used OVS_GUARDED; /* Last use; time created if never used. */
 };
 
 void ofproto_rule_ref(struct rule *);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index a9cf221..b22e6d9 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1935,6 +1935,46 @@ int
 ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
     OVS_EXCLUDED(ofproto_mutex)
 {
+    /* Optimize for the most common case of a repeated learn action.
+     * If an identical flow already exists we only need to update its
+     * 'modified' time. */
+    if (fm->command == OFPFC_MODIFY_STRICT && fm->table_id != OFPTT_ALL) {
+        struct oftable *table = &ofproto->tables[fm->table_id];
+        struct cls_rule match_rule;
+        struct rule *rule;
+        bool done = false;
+
+        cls_rule_init(&match_rule, &fm->match, fm->priority);
+        fat_rwlock_rdlock(&table->cls.rwlock);
+        rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls,
+                                                               &match_rule));
+        if (rule) {
+            /* Reading many of the rule fields and writing on 'modified'
+             * requires the rule->mutex.  Also, rule->actions may change
+             * if rule->mutex is not held. */
+            ovs_mutex_lock(&rule->mutex);
+            if (rule->idle_timeout == fm->idle_timeout
+                && rule->hard_timeout == fm->hard_timeout
+                && rule->flags == (fm->flags & OFPUTIL_FF_STATE)
+                && !(fm->flags & OFPUTIL_FF_RESET_COUNTS)
+                && (!fm->modify_cookie || (fm->new_cookie == rule->flow_cookie))
+                && ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
+                                 rule->actions->ofpacts,
+                                 rule->actions->ofpacts_len)) {
+                /* Rule already exists and need not change, only update the
+                   modified timestamp. */
+                rule->modified = time_msec();
+                done = true;
+            }
+            ovs_mutex_unlock(&rule->mutex);
+        }
+        fat_rwlock_unlock(&table->cls.rwlock);
+
+        if (done) {
+            return 0;
+        }
+    }
+
     return handle_flow_mod__(ofproto, NULL, fm, NULL);
 }
 
@@ -6192,10 +6232,13 @@ ofopgroup_complete(struct ofopgroup *group)
             if (!op->error) {
                 long long int now = time_msec();
 
+                ovs_mutex_lock(&rule->mutex);
                 rule->modified = now;
                 if (op->type == OFOPERATION_REPLACE) {
                     rule->created = rule->used = now;
                 }
+                ovs_mutex_unlock(&rule->mutex);
+
             } else {
                 ofproto_rule_change_cookie(ofproto, rule, op->flow_cookie);
                 ovs_mutex_lock(&rule->mutex);
-- 
1.7.10.4