Re: [libvirt] [PATCH 14/17] nwfilter: Add @refs logic to __virNWFilterObj

16 Jul 2017

On 07/13/2017 10:40 AM, Michal Privoznik wrote:
...
On 06/02/2017 12:25 PM, John Ferlan wrote:
...
"Simple" conversion to the virObjectLockable object isn't quite possible
yet because the nwfilter lock requires usage of recursive locking due to
algorithms handing filter "<rule>"'s and "<filterref>"'s. The list search
logic would also benefit from using hash tables for lookups. So this patch
is step 1 in the process - add the @refs to _virNWFilterObj and modify the
algorithms to use (a temporary) virNWFilterObj{Ref|Unref} in order to set
things up for the list logic to utilize virObjectLockable hash tables.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virnwfilterobj.c              | 75 ++++++++++++++++++++++++++--------
 src/conf/virnwfilterobj.h              | 15 ++++---
 src/libvirt_private.syms               |  5 ++-
 src/nwfilter/nwfilter_driver.c         | 13 +++---
 src/nwfilter/nwfilter_gentech_driver.c | 11 +++--
 5 files changed, 80 insertions(+), 39 deletions(-)
FWIW: I've pushed 1, 4, and 8-13 since they were ACK. Since 2, 3, and
5-7 are being dropped and I want to insert a revert prior to here
(separate patch posted) - I'll respond to the queries from 14-17, but
they will get reposted as a v2 once the revert is in.
...
...

diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index b21b570..99be59c 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -23,6 +23,7 @@
 #include "datatypes.h"
#include "viralloc.h"
+#include "viratomic.h"
 #include "virerror.h"
 #include "virfile.h"
 #include "virlog.h"
@@ -33,8 +34,15 @@
VIR_LOG_INIT("conf.virnwfilterobj");
+static void
+virNWFilterObjLock(virNWFilterObjPtr obj);
+
+static void
+virNWFilterObjUnlock(virNWFilterObjPtr obj);
+
 struct _virNWFilterObj {
     virMutex lock;
+    int refs;
bool wantRemoved;
@@ -67,11 +75,24 @@ virNWFilterObjNew(virNWFilterDefPtr def)
virNWFilterObjLock(obj);
     obj->def = def;
+    virAtomicIntSet(&obj->refs, 1);
Technically, this doesn't need to be virAtomic. Bare assignment would
work as: a) the object is locked at this point, b) there's no other
reference to the object (we are just creating the first one). But I'm
fine with leaving this as is, just wanted to point out my comment.
True, but in order to "show" the progression from point A to point B
*and* because of that recursive locking algorithm currently in place, I
essentially lifted code from virobject.c (this is from virObjectNew and
virObjectLockableNew).
...
...
return obj;
 }
+void
+virNWFilterObjEndAPI(virNWFilterObjPtr *obj)
+{
+    if (!*obj)
+        return;
+
+    virNWFilterObjUnlock(*obj);
+    virNWFilterObjUnref(*obj);
+    *obj = NULL;
+}
+
+
 virNWFilterDefPtr
 virNWFilterObjGetDef(virNWFilterObjPtr obj)
 {
@@ -109,12 +130,33 @@ virNWFilterObjFree(virNWFilterObjPtr obj)
 }
+virNWFilterObjPtr
+virNWFilterObjRef(virNWFilterObjPtr obj)
+{
+    if (obj)
+        virAtomicIntInc(&obj->refs);
+    return obj;
+}
+
+
+bool
+virNWFilterObjUnref(virNWFilterObjPtr obj)
+{
+    bool lastRef;
+    if (obj)
+        return false;
This can hardly work. The condition needs to be inverted.
Oh right....  Good eyes!  My testing wouldn't see it because I tested
the whole series which would essentially replace this.
...
...
+    if ((lastRef = virAtomicIntDecAndTest(&obj->refs)))
+        virNWFilterObjFree(obj);
+    return !lastRef;
+}
+
+
 void
 virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
 {
     size_t i;
     for (i = 0; i < nwfilters->count; i++)
-        virNWFilterObjFree(nwfilters->objs[i]);
+        virNWFilterObjUnref(nwfilters->objs[i]);
     VIR_FREE(nwfilters->objs);
     VIR_FREE(nwfilters);
 }
@@ -143,7 +185,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
         virNWFilterObjLock(nwfilters->objs[i]);
         if (nwfilters->objs[i] == obj) {
             virNWFilterObjUnlock(nwfilters->objs[i]);
-            virNWFilterObjFree(nwfilters->objs[i]);
+            virNWFilterObjUnref(nwfilters->objs[i]);
VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
             break;
@@ -166,7 +208,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
         virNWFilterObjLock(obj);
         def = obj->def;
         if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
-            return obj;
+            return virNWFilterObjRef(obj);
         virNWFilterObjUnlock(obj);
     }
@@ -187,7 +229,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
         virNWFilterObjLock(obj);
         def = obj->def;
         if (STREQ_NULLABLE(def->name, name))
-            return obj;
+            return virNWFilterObjRef(obj);
         virNWFilterObjUnlock(obj);
     }
@@ -210,7 +252,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
     if (virNWFilterObjWantRemoved(obj)) {
         virReportError(VIR_ERR_NO_NWFILTER,
                        _("Filter '%s' is in use."), filtername);
-        virNWFilterObjUnlock(obj);
+        virNWFilterObjEndAPI(&obj);
         return NULL;
Can we remove this "return NULL" line and rely on "return obj" which
follow immediately (not to be seen in the context here though)?
Sure that's fine, same result.
...
...
}
@@ -245,7 +287,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
             if (obj) {
                 rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def,
                                                       filtername);
-                virNWFilterObjUnlock(obj);
+                virNWFilterObjEndAPI(&obj);
                 if (rc < 0)
                     break;
             }
@@ -338,10 +380,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
                            _("filter with same UUID but different name "
                              "('%s') already exists"),
                            objdef->name);
-            virNWFilterObjUnlock(obj);
+            virNWFilterObjEndAPI(&obj);
             return NULL;
         }
-        virNWFilterObjUnlock(obj);
+        virNWFilterObjEndAPI(&obj);
     } else {
         if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
             char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -351,7 +393,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("filter '%s' already exists with uuid %s"),
                            def->name, uuidstr);
-            virNWFilterObjUnlock(obj);
+            virNWFilterObjEndAPI(&obj);
             return NULL;
         }
     }
@@ -362,7 +404,6 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
         return NULL;
     }
-
     if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
objdef = obj->def;
I've got an unrelated question: here, after this line you can find the
following code:
if (virNWFilterDefEqual(def, objdef, false)) {
            virNWFilterDefFree(objdef);
            obj->def = def;
            return obj;
        }
Firstly, I think we can s/false/true/ because if UUIDs were not the
same, we would have errored out way sooner. But more importantly, if
definition is equal what's the point in replacing it?
Let's see, looks like commit id '46a811db07' added the if we cannot find
by UUID, then let's find by Name and if the name exists, then cause
failure because we have a defined Name with a different UUID.

But it didn't adjust this algorithm; however, rather than change to
passing true, since this is the only caller, the @cmpUUIDs should be
removed from virNWFilterDefEqual.  In a separate followup patch of course.

As noted above - I'll fix stuff up here, add the patch, and post a v2

Tks -

John
...
...
@@ -376,7 +417,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
         /* trigger the update on VMs referencing the filter */
         if (virNWFilterTriggerVMFilterRebuild() < 0) {
             obj->newDef = NULL;
-            virNWFilterObjUnlock(obj);
+            virNWFilterObjEndAPI(&obj);
             return NULL;
         }
@@ -397,12 +438,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
     if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0)
         goto error;
-    return obj;
+    return virNWFilterObjRef(obj);
error:
     obj->def = NULL;
     virNWFilterObjUnlock(obj);
-    virNWFilterObjFree(obj);
+    virNWFilterObjUnref(obj);
     return NULL;
 }
@@ -600,8 +641,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
             continue;
obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name);
-        if (obj)
-            virNWFilterObjUnlock(obj);
+
+        virNWFilterObjEndAPI(&obj);
     }
VIR_DIR_CLOSE(dir);
@@ -609,14 +650,14 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
 }
-void
+static void
 virNWFilterObjLock(virNWFilterObjPtr obj)
 {
     virMutexLock(&obj->lock);
 }
-void
+static void
 virNWFilterObjUnlock(virNWFilterObjPtr obj)
 {
     virMutexUnlock(&obj->lock);
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
index 85c8ead..31aa345 100644
--- a/src/conf/virnwfilterobj.h
+++ b/src/conf/virnwfilterobj.h
@@ -41,6 +41,9 @@ struct _virNWFilterDriverState {
     bool watchingFirewallD;
 };
+void
+virNWFilterObjEndAPI(virNWFilterObjPtr *obj);
+
 virNWFilterDefPtr
 virNWFilterObjGetDef(virNWFilterObjPtr obj);
@@ -50,6 +53,12 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj);
 bool
 virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
+virNWFilterObjPtr
+virNWFilterObjRef(virNWFilterObjPtr obj);
+
+bool
+virNWFilterObjUnref(virNWFilterObjPtr obj);
+
ACK
Michal


    