[libvirt] [PATCH v4 6/7] remote: shrink the critical sections

Marc Hartmayer posted 7 patches 6 years, 2 months ago
[libvirt] [PATCH v4 6/7] remote: shrink the critical sections
Posted by Marc Hartmayer 6 years, 2 months ago
To free the structs and save the error, it is not necessary to hold @priv->lock,
therefore move these parts after the mutex unlock.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 9dc2083d715a..6ece51c2889d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4293,10 +4293,10 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -4342,9 +4342,9 @@ remoteDispatchConnectDomainEventDeregister(virNetServerPtr server G_GNUC_UNUSED,
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -4522,10 +4522,10 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -4598,11 +4598,11 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(dom);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -4657,9 +4657,9 @@ remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server G_GNUC_UNUS
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -4702,9 +4702,9 @@ remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server G_G
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6081,11 +6081,11 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server G_GNUC_UNUSE
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(net);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6128,9 +6128,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server G_GNUC_UNU
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6202,11 +6202,11 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server G_GNUC_U
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(pool);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6248,9 +6248,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr server G_GNUC
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6322,11 +6322,11 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server G_GNUC_UN
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(dev);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6368,9 +6368,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr server G_GNUC_
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6442,11 +6442,11 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(secret);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6488,9 +6488,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server G_GNUC_UNUS
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6558,11 +6558,11 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server G_GNUC_UNUS
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(dom);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
@@ -6606,9 +6606,9 @@ qemuDispatchConnectDomainMonitorEventDeregister(virNetServerPtr server G_GNUC_UN
     rv = 0;
 
  cleanup:
+    virMutexUnlock(&priv->lock);
     if (rv < 0)
         virNetMessageSaveError(rerr);
-    virMutexUnlock(&priv->lock);
     return rv;
 }
 
-- 
2.21.0


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

Re: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
Posted by Cole Robinson 6 years, 1 month ago
On 11/14/19 12:44 PM, Marc Hartmayer wrote:
> To free the structs and save the error, it is not necessary to hold @priv->lock,
> therefore move these parts after the mutex unlock.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
>  1 file changed, 16 insertions(+), 16 deletions(-)

Reviewed-by: Cole Robinson <crobinso@redhat.com>

Do I understand correctly that 1,3-5 are all independent and can be
pushed separately? If so I will do that tomorrow. I'm doing some
archaeology on patch #2

- Cole

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

Re: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
Posted by Marc Hartmayer 6 years, 1 month ago
On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson <crobinso@redhat.com> wrote:
> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
>> To free the structs and save the error, it is not necessary to hold @priv->lock,
>> therefore move these parts after the mutex unlock.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>
> Do I understand correctly that 1,3-5 are all independent and can be
> pushed separately? If so I will do that tomorrow. I'm doing some
> archaeology on patch #2

1, 3, and 5 are all independent.

Patch 4 depends on the second patch as
remoteDispatchConnectRegisterCloseCallback uses
virConnectRegisterCloseCallback. Otherwise we would never do the unref
for @client and @program when conn->driver->connectRegisterCloseCallback
was NULL.

>
> - Cole
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
Posted by Cole Robinson 6 years, 1 month ago
On 12/12/19 9:13 AM, Marc Hartmayer wrote:
> On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson <crobinso@redhat.com> wrote:
>> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
>>> To free the structs and save the error, it is not necessary to hold @priv->lock,
>>> therefore move these parts after the mutex unlock.
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> ---
>>>  src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
>>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>>
>> Do I understand correctly that 1,3-5 are all independent and can be
>> pushed separately? If so I will do that tomorrow. I'm doing some
>> archaeology on patch #2
> 
> 1, 3, and 5 are all independent.
> 
> Patch 4 depends on the second patch as
> remoteDispatchConnectRegisterCloseCallback uses
> virConnectRegisterCloseCallback. Otherwise we would never do the unref
> for @client and @program when conn->driver->connectRegisterCloseCallback
> was NULL.

Thanks, I pushed 1, 3, 5, 6. I'll reply to your other message

- Cole

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

Re: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
Posted by Marc Hartmayer 6 years, 1 month ago
On Fri, Dec 13, 2019 at 03:00 PM -0500, Cole Robinson <crobinso@redhat.com> wrote:
> On 12/12/19 9:13 AM, Marc Hartmayer wrote:
>> On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson <crobinso@redhat.com> wrote:
>>> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
>>>> To free the structs and save the error, it is not necessary to hold @priv->lock,
>>>> therefore move these parts after the mutex unlock.
>>>>
>>>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>>> ---
>>>>  src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
>>>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>>>
>>> Do I understand correctly that 1,3-5 are all independent and can be
>>> pushed separately? If so I will do that tomorrow. I'm doing some
>>> archaeology on patch #2
>> 
>> 1, 3, and 5 are all independent.
>> 
>> Patch 4 depends on the second patch as
>> remoteDispatchConnectRegisterCloseCallback uses
>> virConnectRegisterCloseCallback. Otherwise we would never do the unref
>> for @client and @program when conn->driver->connectRegisterCloseCallback
>> was NULL.
>
> Thanks, I pushed 1, 3, 5, 6. I'll reply to your other message
>
> - Cole
>

Thanks.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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