[PATCHv2 0/4] Fix timeout_list handling

[PATCHv2 0/4] Fix timeout_list handling

[Marian Csontos]

Two fixes how timeout_list is handled and two [micro] optimizations.

This should address BZ 1210514 and BZ 1220856.

Marian Csontos (4):
  dmeventd: Fix segfault in _timeout_thread
  dmeventd: Fix registration of existing thread
  dmeventd: Remove an useless _lock_mutex
  dmeventd: Allocate new thread only when needed

 daemons/dmeventd/dmeventd.c | 73 ++++++++++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 28 deletions(-)

-- 
1.8.3.1

[PATCHv2 1/4] dmeventd: Fix segfault in _timeout_thread

[Marian Csontos]

In case thread was found or thread registration failed after
_register_for_timeout, thread_status was kept in timeout_list after
deallocation.
---
This should be the actual fix for the above mentioned BZ 1210514 and BZ 1220856.

*TODO* Any ideas how to test this? Is explicitly messaging dmeventd required?

 daemons/dmeventd/dmeventd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index 1ff5bf9..0f67206 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1081,6 +1081,7 @@ static int _register_for_event(struct message_data *message_data)
 		if ((ret = -_create_thread(thread))) {
 			_unlock_mutex();
 			_do_unregister_device(thread);
+			_unregister_for_timeout(thread);
 			_free_thread_status(thread);
 			goto out;
 		}
@@ -1097,8 +1098,10 @@ static int _register_for_event(struct message_data *message_data)
 	 * Deallocate thread status after releasing
 	 * the lock in case we haven't used it.
 	 */
-	if (thread_new)
+	if (thread_new) {
+		_unregister_for_timeout(thread);
 		_free_thread_status(thread_new);
+	}
 
 	return ret;
 }
-- 
1.8.3.1

[PATCHv2 2/4] dmeventd: Fix registration of existing thread

[Marian Csontos]

The existing code is handling only new registrations correctly. Extending
an existing registration does not register for timeout.
---
If the condition should "never occur" we should either handle it or at least
raise an internal error here as it is likely to occur one day without traces
what happened.

*TODO* Any ideas how to test this? Is explicitly messaging dmeventd required?

 daemons/dmeventd/dmeventd.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index 0f67206..ff11625 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1031,6 +1031,7 @@ static int _register_for_event(struct message_data *message_data)
 	int ret = 0;
 	struct thread_status *thread, *thread_new = NULL;
 	struct dso_data *dso_data;
+	enum dm_event_mask orig_events
 
 	if (!(dso_data = _lookup_dso(message_data)) &&
 	    !(dso_data = _load_dso(message_data))) {
@@ -1087,10 +1088,23 @@ static int _register_for_event(struct message_data *message_data)
 		}
 
 		LINK_THREAD(thread);
+	} else {
+		/* Or event # into events bitfield. */
+		orig_events = thread->events;
+		thread->events |= message_data->events_field;
+		if ((~orig_events & thread->events & DM_EVENT_TIMEOUT)) {
+			_unlock_mutex();
+			if (!(ret = -_register_for_timeout(thread))) {
+				/* In case previous calls failed we do not
+				 * force unregister event. Reset events for
+				 * consistency. */
+				_lock_mutex();
+				thread->events &= ~DM_EVENT_TIMEOUT;
+			} else
+				_lock_mutex();
+		}
 	}
 
-	/* Or event # into events bitfield. */
-	thread->events |= message_data->events_field;
 	_unlock_mutex();
 
       out:
-- 
1.8.3.1

[PATCHv2 2/4] dmeventd: Fix registration of existing thread

[Marian Csontos]

On 09/22/2015 01:16 PM, Marian Csontos wrote:
> The existing code is handling only new registrations correctly. Extending
> an existing registration does not register for timeout.
> ---
> If the condition should "never occur" we should either handle it or at least
> raise an internal error here as it is likely to occur one day without traces
> what happened.
>
> *TODO* Any ideas how to test this? Is explicitly messaging dmeventd required?
>
>   daemons/dmeventd/dmeventd.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
> index 0f67206..ff11625 100644
> --- a/daemons/dmeventd/dmeventd.c
> +++ b/daemons/dmeventd/dmeventd.c
> @@ -1031,6 +1031,7 @@ static int _register_for_event(struct message_data *message_data)
>   	int ret = 0;
>   	struct thread_status *thread, *thread_new = NULL;
>   	struct dso_data *dso_data;
> +	enum dm_event_mask orig_events

Lost a semicolon while moving from below :-/

BTW, are we always declaring variables at the top of a function, right?

>
>   	if (!(dso_data = _lookup_dso(message_data)) &&
>   	    !(dso_data = _load_dso(message_data))) {
> @@ -1087,10 +1088,23 @@ static int _register_for_event(struct message_data *message_data)
>   		}
>
>   		LINK_THREAD(thread);
> +	} else {
> +		/* Or event # into events bitfield. */
> +		orig_events = thread->events;
> +		thread->events |= message_data->events_field;
> +		if ((~orig_events & thread->events & DM_EVENT_TIMEOUT)) {
> +			_unlock_mutex();
> +			if (!(ret = -_register_for_timeout(thread))) {
> +				/* In case previous calls failed we do not
> +				 * force unregister event. Reset events for
> +				 * consistency. */
> +				_lock_mutex();
> +				thread->events &= ~DM_EVENT_TIMEOUT;
> +			} else
> +				_lock_mutex();
> +		}
>   	}
>
> -	/* Or event # into events bitfield. */
> -	thread->events |= message_data->events_field;
>   	_unlock_mutex();
>
>         out:
>

[PATCH 3/4] dmeventd: Remove an useless _lock_mutex

[Marian Csontos]

One of the branches was locking a mutex only to unlock it later.
---
This may be more esthetic than performance issue.

Definitely trading a little performance gain for clarity.

 daemons/dmeventd/dmeventd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index ff11625..82c8cc3 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1088,6 +1088,7 @@ static int _register_for_event(struct message_data *message_data)
 		}
 
 		LINK_THREAD(thread);
+		_unlock_mutex();
 	} else {
 		/* Or event # into events bitfield. */
 		orig_events = thread->events;
@@ -1100,13 +1101,12 @@ static int _register_for_event(struct message_data *message_data)
 				 * consistency. */
 				_lock_mutex();
 				thread->events &= ~DM_EVENT_TIMEOUT;
-			} else
-				_lock_mutex();
-		}
+				_unlock_mutex();
+			}
+		} else
+			_unlock_mutex();
 	}
 
-	_unlock_mutex();
-
       out:
 	/*
 	 * Deallocate thread status after releasing
-- 
1.8.3.1

[PATCH 4/4] dmeventd: Allocate new thread only when needed

[Marian Csontos]

---
Another microoptimization?

Do we want to go through these hoops only to discard results later on?

These calls being called in front had justification earlier on when everything
was done in a single critical section protected by single lock/unlock pair. But
now when we are taking and releasing mutexes several times in one cycle this is
loosing grounds IMHO.

 daemons/dmeventd/dmeventd.c | 46 ++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index 82c8cc3..a4bf76e 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1044,33 +1044,33 @@ static int _register_for_event(struct message_data *message_data)
 		goto out;
 	}
 
-	/* Preallocate thread status struct to avoid deadlock. */
-	if (!(thread_new = _alloc_thread_status(message_data, dso_data))) {
-		stack;
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	if (!_fill_device_data(thread_new)) {
-		stack;
-		ret = -ENODEV;
-		goto out;
-	}
-
-	/* If creation of timeout thread fails (as it may), we fail
-	   here completely. The client is responsible for either
-	   retrying later or trying to register without timeout
-	   events. However, if timeout thread cannot be started, it
-	   usually means we are so starved on resources that we are
-	   almost as good as dead already... */
-	if ((thread_new->events & DM_EVENT_TIMEOUT) &&
-	    (ret = -_register_for_timeout(thread_new)))
-		goto out;
-
 	_lock_mutex();
 	if (!(thread = _lookup_thread_status(message_data))) {
 		_unlock_mutex();
 
+		/* Preallocate thread status struct to avoid deadlock. */
+		if (!(thread_new = _alloc_thread_status(message_data, dso_data))) {
+			stack;
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		if (!_fill_device_data(thread_new)) {
+			stack;
+			ret = -ENODEV;
+			goto out;
+		}
+
+		/* If creation of timeout thread fails (as it may), we fail
+		   here completely. The client is responsible for either
+		   retrying later or trying to register without timeout
+		   events. However, if timeout thread cannot be started, it
+		   usually means we are so starved on resources that we are
+		   almost as good as dead already... */
+		if ((thread_new->events & DM_EVENT_TIMEOUT) &&
+		    (ret = -_register_for_timeout(thread_new)))
+			goto out;
+
 		if (!(ret = _do_register_device(thread_new)))
 			goto out;
 
-- 
1.8.3.1