Re: [PATCH v5 2/3] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/12/2018 04:38 AM, Michal Privoznik wrote:
On 02/09/2018 12:47 PM, Stefan Berger wrote:
On 02/09/2018 01:48 AM, Michal Privoznik wrote:
On 02/08/2018 10:13 PM, Stefan Berger wrote:
On 02/08/2018 08:13 AM, Michal Privoznik wrote:
On 02/06/2018 08:20 PM, John Ferlan wrote:
Implement the self locking object list for nwfilter object lists
that uses two hash tables to store the nwfilter object by UUID or
by Name.

As part of this alter the uuid argument to virNWFilterObjLookupByUUID
to expect an already formatted uuidstr.

Alter the existing list traversal code to implement the hash table
find/lookup/search functionality.

Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
    src/conf/virnwfilterobj.c      | 405
++++++++++++++++++++++++++++-------------
    src/conf/virnwfilterobj.h      |   2 +-
    src/nwfilter/nwfilter_driver.c |   5 +-
    3 files changed, 283 insertions(+), 129 deletions(-)
@@ -425,24 +483,26 @@
virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
            return NULL;
        }
    -
-    /* Get a READ lock and immediately promote to WRITE while we
adjust
-     * data within. */
-    if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
+    /* We're about to make some changes to objects on the list - so
get the
+     * list READ lock in order to Find the object and WRITE lock the
object
+     * since both paths would immediately promote it anyway */
+    virObjectRWLockRead(nwfilters);
+    if ((obj = virNWFilterObjListFindByNameLocked(nwfilters,
def->name))) {
+        virObjectRWLockWrite(obj);
+        virObjectRWUnlock(nwfilters);
              objdef = obj->def;
            if (virNWFilterDefEqual(def, objdef)) {
-            virNWFilterObjPromoteToWrite(obj);
                virNWFilterDefFree(objdef);
                obj->def = def;
                virNWFilterObjDemoteFromWrite(obj);
                return obj;
            }
    -        /* Set newDef and run the trigger with a demoted lock
since it may need
-         * to grab a read lock on this object and promote before
returning. */
-        virNWFilterObjPromoteToWrite(obj);
            obj->newDef = def;
+
+        /* Demote while the trigger runs since it may need to grab a
read
+         * lock on this object and promote before returning. */
            virNWFilterObjDemoteFromWrite(obj);
              /* trigger the update on VMs referencing the filter */
@@ -462,39 +522,113 @@
virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
            return obj;
        }
    +    /* Promote the nwfilters to add a new object */
+    virObjectRWUnlock(nwfilters);
+    virObjectRWLockWrite(nwfilters);
How can this work? What if there's another thread waiting for the write
lock (e.g. just to define an NWFilter)? The Unlock(nwfilters) wakes up
the other thread, it does its job, unlocks the list waking us in turn.
So we lock @nwfilters for writing and add something into the hash table
- without all the checks (e.g. duplicity check) done earlier.
Could we keep the read lock and grab a 2nd lock as a write-lock? It
wouldn't be a virObject call, though.
That is not possible because write & read lock must exclude each other
by definition.
We can grab lock A and then lock B. Lock B would either be a read/write
lock locked as a write lock or would be an exclusive lock. Why would
that not work?
Oh, I'm misunderstanding. What do you mean by locks A and B?
virNWFilterObj and virNWFilterObjList locks or something else?

We could use the lock for recursive locking as we do it now. For 'promoting' to write lock we would grab a 2nd lock that's exclusive.

   Stefan


Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux