[dm-devel] [PATCH v2 04/15] libmultipath: uevent_dispatch(): process uevents one by one

[mwilck@suse.com]

[-- Attachment #1: Type: application/octet-stream, Size: 10407 bytes --]

From: Martin Wilck <mwilck@suse.com>

The main rationale for delaying uevents is that the
uevent dispatcher may have to wait for other threads to release the
vecs lock, may the vecs lock for an extended amount of time, and
even sleep occasionally. By delaying them, we have the chance
to accumulate events for the same path device ("filtering") or
WWID ("merging"), thus avoiding duplicate work if we merge these
into one.

A similar effect can be obtained in the uevent dispatcher itself
by looking for new uevents after each dispatched event, and trying
to merge the newly arrived events with those that remained
in the queue.

When uevq_work is non-empty and we append a list of new events,
we don't need to check the entire list for filterable and mergeable
uevents. uevq_work had been filtered and merged already. So we just
need to check the newly appended events. These must of course be
checked for merges with earlier events, too.

We must deal with some special cases here, like previously merged
uevents being filtered later.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/list.h   |  53 ++++++++++++++++++
 libmultipath/uevent.c | 127 +++++++++++++++++++++++++++++++-----------
 2 files changed, 146 insertions(+), 34 deletions(-)

diff --git a/libmultipath/list.h b/libmultipath/list.h
index ced021f..248f72b 100644
--- a/libmultipath/list.h
+++ b/libmultipath/list.h
@@ -246,6 +246,35 @@ static inline void list_splice_tail_init(struct list_head *list,
 #define list_entry(ptr, type, member) \
 	container_of(ptr, type, member)
 
+
+/**
+ * list_pop - unlink and return the first list element
+ * @head:	the &struct list_head pointer.
+ */
+static inline struct list_head *list_pop(struct list_head *head)
+{
+	struct list_head *tmp;
+
+	if (list_empty(head))
+		return NULL;
+	tmp = head->next;
+	list_del_init(tmp);
+	return tmp;
+}
+
+/**
+ * list_pop_entry - unlink and return the entry of the first list element
+ * @head:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_pop_entry(head, type, member)		\
+({							\
+	struct list_head *__h = list_pop(head);		\
+							\
+	(__h ? container_of(__h, type, member) : NULL);	\
+})
+
 /**
  * list_for_each	-	iterate over a list
  * @pos:	the &struct list_head to use as a loop counter.
@@ -334,6 +363,30 @@ static inline void list_splice_tail_init(struct list_head *list,
 	     &pos->member != (head);                                    \
 	     pos = n, n = list_entry(n->member.prev, typeof(*n), member))
 
+/**
+ * list_for_some_entry - iterate list from the given begin node to the given end node
+ * @pos:	the type * to use as a loop counter.
+ * @from:	the begin node of the iteration.
+ * @to:		the end node of the iteration.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_some_entry(pos, from, to, member)                      \
+	for (pos = list_entry((from)->next, typeof(*pos), member);      \
+	     &pos->member != (to);                                      \
+	     pos = list_entry(pos->member.next, typeof(*pos), member))
+
+/**
+ * list_for_some_entry_reverse - iterate backwards list from the given begin node to the given end node
+ * @pos:	the type * to use as a loop counter.
+ * @from:	the begin node of the iteration.
+ * @to:		the end node of the iteration.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_some_entry_reverse(pos, from, to, member)		\
+	for (pos = list_entry((from)->prev, typeof(*pos), member);      \
+	     &pos->member != (to);                                      \
+	     pos = list_entry(pos->member.prev, typeof(*pos), member))
+
 /**
  * list_for_some_entry_safe - iterate list from the given begin node to the given end node safe against removal of list entry
  * @pos:	the type * to use as a loop counter.
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index fe60ae3..66acd6c 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -306,17 +306,64 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later)
 	return false;
 }
 
+static void uevent_delete_from_list(struct uevent *to_delete,
+				    struct uevent **previous,
+				    struct list_head **old_tail)
+{
+	/*
+	 * "old_tail" is the list_head before the last list element to which
+	 * the caller iterates (the list anchor if the caller iterates over
+	 * the entire list). If this element is removed (which can't happen
+	 * for the anchor), "old_tail" must be moved. It can happen that
+	 * "old_tail" ends up pointing at the anchor.
+	 */
+	if (*old_tail == &to_delete->node)
+		*old_tail = to_delete->node.prev;
+
+	list_del_init(&to_delete->node);
+
+	/*
+	 * The "to_delete" uevent has been merged with other uevents
+	 * previously. Re-insert them into the list, at the point we're
+	 * currently at. This must be done after the list_del_init() above,
+	 * otherwise previous->next would still point to to_delete.
+	 */
+	if (!list_empty(&to_delete->merge_node)) {
+		struct uevent *last = list_entry(to_delete->merge_node.prev,
+						 typeof(*last), node);
+
+		list_splice(&to_delete->merge_node, &(*previous)->node);
+		*previous = last;
+	}
+	if (to_delete->udev)
+		udev_device_unref(to_delete->udev);
+
+	free(to_delete);
+}
+
+/*
+ * Use this function to delete events that are known not to
+ * be equal to old_tail, and have an empty merge_node list.
+ * For others, use uevent_delete_from_list().
+ */
+static void uevent_delete_simple(struct uevent *to_delete)
+{
+	list_del_init(&to_delete->node);
+
+	if (to_delete->udev)
+		udev_device_unref(to_delete->udev);
+
+	free(to_delete);
+}
+
 static void
-uevent_prepare(struct list_head *tmpq)
+uevent_prepare(struct list_head *tmpq, const struct list_head *stop)
 {
 	struct uevent *uev, *tmp;
 
-	list_for_each_entry_reverse_safe(uev, tmp, tmpq, node) {
+	list_for_some_entry_reverse_safe(uev, tmp, tmpq, stop, node) {
 		if (uevent_can_discard(uev)) {
-			list_del_init(&uev->node);
-			if (uev->udev)
-				udev_device_unref(uev->udev);
-			free(uev);
+			uevent_delete_simple(uev);
 			continue;
 		}
 
@@ -327,7 +374,7 @@ uevent_prepare(struct list_head *tmpq)
 }
 
 static void
-uevent_filter(struct uevent *later, struct list_head *tmpq)
+uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
 {
 	struct uevent *earlier, *tmp;
 
@@ -341,16 +388,13 @@ uevent_filter(struct uevent *later, struct list_head *tmpq)
 				earlier->kernel, earlier->action,
 				later->kernel, later->action);
 
-			list_del_init(&earlier->node);
-			if (earlier->udev)
-				udev_device_unref(earlier->udev);
-			free(earlier);
+			uevent_delete_from_list(earlier, &tmp, stop);
 		}
 	}
 }
 
 static void
-uevent_merge(struct uevent *later, struct list_head *tmpq)
+uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
 {
 	struct uevent *earlier, *tmp;
 
@@ -365,37 +409,42 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
 				earlier->action, earlier->kernel, earlier->wwid,
 				later->action, later->kernel, later->wwid);
 
+			/* See comment in uevent_delete_from_list() */
+			if (&earlier->node == *stop)
+				*stop = earlier->node.prev;
+
 			list_move(&earlier->node, &later->merge_node);
+			list_splice_init(&earlier->merge_node,
+					 &later->merge_node);
 		}
 	}
 }
 
 static void
-merge_uevq(struct list_head *tmpq)
+merge_uevq(struct list_head *tmpq, struct list_head *stop)
 {
 	struct uevent *later;
 
-	uevent_prepare(tmpq);
-	list_for_each_entry_reverse(later, tmpq, node) {
-		uevent_filter(later, tmpq);
+	uevent_prepare(tmpq, stop);
+	list_for_some_entry_reverse(later, tmpq, stop, node) {
+		uevent_filter(later, tmpq, &stop);
 		if(uevent_need_merge())
-			uevent_merge(later, tmpq);
+			uevent_merge(later, tmpq, &stop);
 	}
 }
 
 static void
 service_uevq(struct list_head *tmpq)
 {
-	struct uevent *uev, *tmp;
+	struct uevent *uev = list_pop_entry(tmpq, typeof(*uev), node);
 
-	list_for_each_entry_safe(uev, tmp, tmpq, node) {
-		list_del_init(&uev->node);
-
-		pthread_cleanup_push(cleanup_uev, uev);
-		if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
-			condlog(0, "uevent trigger error");
-		pthread_cleanup_pop(1);
-	}
+	if (uev == NULL)
+		return;
+	condlog(4, "servicing uevent '%s %s'", uev->action, uev->kernel);
+	pthread_cleanup_push(cleanup_uev, uev);
+	if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
+		condlog(0, "uevent trigger error");
+	pthread_cleanup_pop(1);
 }
 
 static void uevent_cleanup(void *arg)
@@ -432,33 +481,43 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
 int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		    void * trigger_data)
 {
+	LIST_HEAD(uevq_work);
+
 	my_uev_trigger = uev_trigger;
 	my_trigger_data = trigger_data;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 
+	pthread_cleanup_push(cleanup_uevq, &uevq_work);
 	while (1) {
-		LIST_HEAD(uevq_tmp);
+		struct list_head *stop;
 
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
-		servicing_uev = 0;
 
-		while (list_empty(&uevq))
+		servicing_uev = !list_empty(&uevq_work);
+
+		while (list_empty(&uevq_work) && list_empty(&uevq))
 			pthread_cond_wait(uev_condp, uevq_lockp);
 
 		servicing_uev = 1;
-		list_splice_init(&uevq, &uevq_tmp);
+		/*
+		 * "stop" is the list element towards which merge_uevq()
+		 * will iterate: the last element of uevq_work before
+		 * appending new uevents. If uveq_is empty, uevq_work.prev
+		 * equals &uevq_work, which is what we need.
+		 */
+		stop = uevq_work.prev;
+		list_splice_tail_init(&uevq, &uevq_work);
 		pthread_cleanup_pop(1);
 
 		if (!my_uev_trigger)
 			break;
 
-		pthread_cleanup_push(cleanup_uevq, &uevq_tmp);
-		merge_uevq(&uevq_tmp);
-		service_uevq(&uevq_tmp);
-		pthread_cleanup_pop(1);
+		merge_uevq(&uevq_work, stop);
+		service_uevq(&uevq_work);
 	}
+	pthread_cleanup_pop(1);
 	condlog(3, "Terminating uev service queue");
 	return 0;
 }
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel