All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: stefan.wahren@i2se.com, eric@anholt.net, dave.stevenson@raspberrypi.org
Cc: nsaenzjulienne@suse.de, linux-rpi-kernel@lists.infradead.org,
	gregkh@linuxfoundation.org, linux-arm-kernel@lists.infradead.org,
	devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: [PATCH RFC 11/18] staging: vchiq_arm: use completions instead of semaphores
Date: Fri, 26 Oct 2018 15:48:06 +0200	[thread overview]
Message-ID: <20181026134813.7775-12-nsaenzjulienne@suse.de> (raw)
In-Reply-To: <20181026134813.7775-1-nsaenzjulienne@suse.de>

It is preferred in the kernel to avoid using semaphores to wait for
events, as they are optimised for the opposite situation; where the
common case is that they are available and may block only occasionally.
FYI see this thread: https://lkml.org/lkml/2008/4/11/323.

Also completions are semantically more explicit in this case.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 58 ++++++++++---------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 1cdfdb714abc..463d0eec2e96 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -44,7 +44,7 @@
 #include <linux/highmem.h>
 #include <linux/pagemap.h>
 #include <linux/bug.h>
-#include <linux/semaphore.h>
+#include <linux/completion.h>
 #include <linux/list.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -121,9 +121,9 @@ typedef struct user_service_struct {
 	int message_available_pos;
 	int msg_insert;
 	int msg_remove;
-	struct semaphore insert_event;
-	struct semaphore remove_event;
-	struct semaphore close_event;
+	struct completion insert_event;
+	struct completion remove_event;
+	struct completion close_event;
 	VCHIQ_HEADER_T * msg_queue[MSG_QUEUE_SIZE];
 } USER_SERVICE_T;
 
@@ -138,8 +138,8 @@ struct vchiq_instance_struct {
 	VCHIQ_COMPLETION_DATA_T completions[MAX_COMPLETIONS];
 	int completion_insert;
 	int completion_remove;
-	struct semaphore insert_event;
-	struct semaphore remove_event;
+	struct completion insert_event;
+	struct completion remove_event;
 	struct mutex completion_mutex;
 
 	int connected;
@@ -562,7 +562,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
 		vchiq_log_trace(vchiq_arm_log_level,
 			"%s - completion queue full", __func__);
 		DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
-		if (down_interruptible(&instance->remove_event) != 0) {
+		if (wait_for_completion_interruptible(
+					&instance->remove_event)) {
 			vchiq_log_info(vchiq_arm_log_level,
 				"service_callback interrupted");
 			return VCHIQ_RETRY;
@@ -600,7 +601,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
 	insert++;
 	instance->completion_insert = insert;
 
-	up(&instance->insert_event);
+	complete(&instance->insert_event);
 
 	return VCHIQ_SUCCESS;
 }
@@ -673,7 +674,8 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
 			}
 
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-			if (down_interruptible(&user_service->remove_event)
+			if (wait_for_completion_interruptible(
+						&user_service->remove_event)
 				!= 0) {
 				vchiq_log_info(vchiq_arm_log_level,
 					"%s interrupted", __func__);
@@ -705,7 +707,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
 		}
 
 		spin_unlock(&msg_queue_spinlock);
-		up(&user_service->insert_event);
+		complete(&user_service->insert_event);
 
 		header = NULL;
 	}
@@ -745,7 +747,7 @@ static void close_delivered(USER_SERVICE_T *user_service)
 		unlock_service(user_service->service);
 
 		/* Wake the user-thread blocked in close_ or remove_service */
-		up(&user_service->close_event);
+		complete(&user_service->close_event);
 
 		user_service->close_pending = 0;
 	}
@@ -867,7 +869,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (status == VCHIQ_SUCCESS) {
 			/* Wake the completion thread and ask it to exit */
 			instance->closing = 1;
-			up(&instance->insert_event);
+			complete(&instance->insert_event);
 		}
 
 		break;
@@ -948,9 +950,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 				instance->completion_remove - 1;
 			user_service->msg_insert = 0;
 			user_service->msg_remove = 0;
-			sema_init(&user_service->insert_event, 0);
-			sema_init(&user_service->remove_event, 0);
-			sema_init(&user_service->close_event, 0);
+			init_completion(&user_service->insert_event);
+			init_completion(&user_service->remove_event);
+			init_completion(&user_service->close_event);
 
 			if (args.is_open) {
 				status = vchiq_open_service_internal
@@ -1007,7 +1009,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		   has been closed until the client library calls the
 		   CLOSE_DELIVERED ioctl, signalling close_event. */
 		if (user_service->close_pending &&
-			down_interruptible(&user_service->close_event))
+			wait_for_completion_interruptible(
+				&user_service->close_event))
 			status = VCHIQ_RETRY;
 		break;
 	}
@@ -1182,7 +1185,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 			DEBUG_TRACE(AWAIT_COMPLETION_LINE);
 			mutex_unlock(&instance->completion_mutex);
-			rc = down_interruptible(&instance->insert_event);
+			rc = wait_for_completion_interruptible(
+						&instance->insert_event);
 			mutex_lock(&instance->completion_mutex);
 			if (rc != 0) {
 				DEBUG_TRACE(AWAIT_COMPLETION_LINE);
@@ -1310,7 +1314,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 
 		if (ret != 0)
-			up(&instance->remove_event);
+			complete(&instance->remove_event);
 		mutex_unlock(&instance->completion_mutex);
 		DEBUG_TRACE(AWAIT_COMPLETION_LINE);
 	} break;
@@ -1350,8 +1354,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			do {
 				spin_unlock(&msg_queue_spinlock);
 				DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
-				if (down_interruptible(
-					&user_service->insert_event) != 0) {
+				if (wait_for_completion_interruptible(
+					&user_service->insert_event)) {
 					vchiq_log_info(vchiq_arm_log_level,
 						"DEQUEUE_MESSAGE interrupted");
 					ret = -EINTR;
@@ -1373,7 +1377,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		user_service->msg_remove++;
 		spin_unlock(&msg_queue_spinlock);
 
-		up(&user_service->remove_event);
+		complete(&user_service->remove_event);
 		if (header == NULL)
 			ret = -ENOTCONN;
 		else if (header->size <= args.bufsize) {
@@ -1979,8 +1983,8 @@ vchiq_open(struct inode *inode, struct file *file)
 
 		vchiq_debugfs_add_instance(instance);
 
-		sema_init(&instance->insert_event, 0);
-		sema_init(&instance->remove_event, 0);
+		init_completion(&instance->insert_event);
+		init_completion(&instance->remove_event);
 		mutex_init(&instance->completion_mutex);
 		mutex_init(&instance->bulk_waiter_list_mutex);
 		INIT_LIST_HEAD(&instance->bulk_waiter_list);
@@ -2033,12 +2037,12 @@ vchiq_release(struct inode *inode, struct file *file)
 
 		/* Wake the completion thread and ask it to exit */
 		instance->closing = 1;
-		up(&instance->insert_event);
+		complete(&instance->insert_event);
 
 		mutex_unlock(&instance->completion_mutex);
 
 		/* Wake the slot handler if the completion queue is full. */
-		up(&instance->remove_event);
+		complete(&instance->remove_event);
 
 		/* Mark all services for termination... */
 		i = 0;
@@ -2047,7 +2051,7 @@ vchiq_release(struct inode *inode, struct file *file)
 			USER_SERVICE_T *user_service = service->base.userdata;
 
 			/* Wake the slot handler if the msg queue is full. */
-			up(&user_service->remove_event);
+			complete(&user_service->remove_event);
 
 			vchiq_terminate_service_internal(service);
 			unlock_service(service);
@@ -2103,7 +2107,7 @@ vchiq_release(struct inode *inode, struct file *file)
 
 				/* Wake any blocked user-thread */
 				if (instance->use_close_delivered)
-					up(&user_service->close_event);
+					complete(&user_service->close_event);
 				unlock_service(service);
 			}
 			instance->completion_remove++;
-- 
2.19.1


WARNING: multiple messages have this Message-ID
From: nsaenzjulienne@suse.de (Nicolas Saenz Julienne)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH RFC 11/18] staging: vchiq_arm: use completions instead of semaphores
Date: Fri, 26 Oct 2018 15:48:06 +0200	[thread overview]
Message-ID: <20181026134813.7775-12-nsaenzjulienne@suse.de> (raw)
In-Reply-To: <20181026134813.7775-1-nsaenzjulienne@suse.de>

It is preferred in the kernel to avoid using semaphores to wait for
events, as they are optimised for the opposite situation; where the
common case is that they are available and may block only occasionally.
FYI see this thread: https://lkml.org/lkml/2008/4/11/323.

Also completions are semantically more explicit in this case.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 58 ++++++++++---------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 1cdfdb714abc..463d0eec2e96 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -44,7 +44,7 @@
 #include <linux/highmem.h>
 #include <linux/pagemap.h>
 #include <linux/bug.h>
-#include <linux/semaphore.h>
+#include <linux/completion.h>
 #include <linux/list.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -121,9 +121,9 @@ typedef struct user_service_struct {
 	int message_available_pos;
 	int msg_insert;
 	int msg_remove;
-	struct semaphore insert_event;
-	struct semaphore remove_event;
-	struct semaphore close_event;
+	struct completion insert_event;
+	struct completion remove_event;
+	struct completion close_event;
 	VCHIQ_HEADER_T * msg_queue[MSG_QUEUE_SIZE];
 } USER_SERVICE_T;
 
@@ -138,8 +138,8 @@ struct vchiq_instance_struct {
 	VCHIQ_COMPLETION_DATA_T completions[MAX_COMPLETIONS];
 	int completion_insert;
 	int completion_remove;
-	struct semaphore insert_event;
-	struct semaphore remove_event;
+	struct completion insert_event;
+	struct completion remove_event;
 	struct mutex completion_mutex;
 
 	int connected;
@@ -562,7 +562,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
 		vchiq_log_trace(vchiq_arm_log_level,
 			"%s - completion queue full", __func__);
 		DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
-		if (down_interruptible(&instance->remove_event) != 0) {
+		if (wait_for_completion_interruptible(
+					&instance->remove_event)) {
 			vchiq_log_info(vchiq_arm_log_level,
 				"service_callback interrupted");
 			return VCHIQ_RETRY;
@@ -600,7 +601,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
 	insert++;
 	instance->completion_insert = insert;
 
-	up(&instance->insert_event);
+	complete(&instance->insert_event);
 
 	return VCHIQ_SUCCESS;
 }
@@ -673,7 +674,8 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
 			}
 
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-			if (down_interruptible(&user_service->remove_event)
+			if (wait_for_completion_interruptible(
+						&user_service->remove_event)
 				!= 0) {
 				vchiq_log_info(vchiq_arm_log_level,
 					"%s interrupted", __func__);
@@ -705,7 +707,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
 		}
 
 		spin_unlock(&msg_queue_spinlock);
-		up(&user_service->insert_event);
+		complete(&user_service->insert_event);
 
 		header = NULL;
 	}
@@ -745,7 +747,7 @@ static void close_delivered(USER_SERVICE_T *user_service)
 		unlock_service(user_service->service);
 
 		/* Wake the user-thread blocked in close_ or remove_service */
-		up(&user_service->close_event);
+		complete(&user_service->close_event);
 
 		user_service->close_pending = 0;
 	}
@@ -867,7 +869,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (status == VCHIQ_SUCCESS) {
 			/* Wake the completion thread and ask it to exit */
 			instance->closing = 1;
-			up(&instance->insert_event);
+			complete(&instance->insert_event);
 		}
 
 		break;
@@ -948,9 +950,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 				instance->completion_remove - 1;
 			user_service->msg_insert = 0;
 			user_service->msg_remove = 0;
-			sema_init(&user_service->insert_event, 0);
-			sema_init(&user_service->remove_event, 0);
-			sema_init(&user_service->close_event, 0);
+			init_completion(&user_service->insert_event);
+			init_completion(&user_service->remove_event);
+			init_completion(&user_service->close_event);
 
 			if (args.is_open) {
 				status = vchiq_open_service_internal
@@ -1007,7 +1009,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		   has been closed until the client library calls the
 		   CLOSE_DELIVERED ioctl, signalling close_event. */
 		if (user_service->close_pending &&
-			down_interruptible(&user_service->close_event))
+			wait_for_completion_interruptible(
+				&user_service->close_event))
 			status = VCHIQ_RETRY;
 		break;
 	}
@@ -1182,7 +1185,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 			DEBUG_TRACE(AWAIT_COMPLETION_LINE);
 			mutex_unlock(&instance->completion_mutex);
-			rc = down_interruptible(&instance->insert_event);
+			rc = wait_for_completion_interruptible(
+						&instance->insert_event);
 			mutex_lock(&instance->completion_mutex);
 			if (rc != 0) {
 				DEBUG_TRACE(AWAIT_COMPLETION_LINE);
@@ -1310,7 +1314,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 
 		if (ret != 0)
-			up(&instance->remove_event);
+			complete(&instance->remove_event);
 		mutex_unlock(&instance->completion_mutex);
 		DEBUG_TRACE(AWAIT_COMPLETION_LINE);
 	} break;
@@ -1350,8 +1354,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			do {
 				spin_unlock(&msg_queue_spinlock);
 				DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
-				if (down_interruptible(
-					&user_service->insert_event) != 0) {
+				if (wait_for_completion_interruptible(
+					&user_service->insert_event)) {
 					vchiq_log_info(vchiq_arm_log_level,
 						"DEQUEUE_MESSAGE interrupted");
 					ret = -EINTR;
@@ -1373,7 +1377,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		user_service->msg_remove++;
 		spin_unlock(&msg_queue_spinlock);
 
-		up(&user_service->remove_event);
+		complete(&user_service->remove_event);
 		if (header == NULL)
 			ret = -ENOTCONN;
 		else if (header->size <= args.bufsize) {
@@ -1979,8 +1983,8 @@ vchiq_open(struct inode *inode, struct file *file)
 
 		vchiq_debugfs_add_instance(instance);
 
-		sema_init(&instance->insert_event, 0);
-		sema_init(&instance->remove_event, 0);
+		init_completion(&instance->insert_event);
+		init_completion(&instance->remove_event);
 		mutex_init(&instance->completion_mutex);
 		mutex_init(&instance->bulk_waiter_list_mutex);
 		INIT_LIST_HEAD(&instance->bulk_waiter_list);
@@ -2033,12 +2037,12 @@ vchiq_release(struct inode *inode, struct file *file)
 
 		/* Wake the completion thread and ask it to exit */
 		instance->closing = 1;
-		up(&instance->insert_event);
+		complete(&instance->insert_event);
 
 		mutex_unlock(&instance->completion_mutex);
 
 		/* Wake the slot handler if the completion queue is full. */
-		up(&instance->remove_event);
+		complete(&instance->remove_event);
 
 		/* Mark all services for termination... */
 		i = 0;
@@ -2047,7 +2051,7 @@ vchiq_release(struct inode *inode, struct file *file)
 			USER_SERVICE_T *user_service = service->base.userdata;
 
 			/* Wake the slot handler if the msg queue is full. */
-			up(&user_service->remove_event);
+			complete(&user_service->remove_event);
 
 			vchiq_terminate_service_internal(service);
 			unlock_service(service);
@@ -2103,7 +2107,7 @@ vchiq_release(struct inode *inode, struct file *file)
 
 				/* Wake any blocked user-thread */
 				if (instance->use_close_delivered)
-					up(&user_service->close_event);
+					complete(&user_service->close_event);
 				unlock_service(service);
 			}
 			instance->completion_remove++;
-- 
2.19.1

  parent reply	other threads:[~2018-10-26 13:48 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-26 13:47 [PATCH RFC 00/18] staging: vchiq: remove dead code & misc fixes Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 01/18] staging: vchiq_core: rework vchiq_get_config Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 02/18] staging: vchiq_arm: rework close/remove_service IOCTLS Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 03/18] staging: vchiq_shim: delete vchi_service_create Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 04/18] stagning: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 05/18] staging: vchiq_arm: get rid of vchi_mh.h Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 06/18] staging: vchiq_arm: rework vchiq_ioc_copy_element_data Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 07/18] staging: vchiq-core: get rid of is_master distinction Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 08/18] staging: vchiq_core: remove unnecessary safety checks in vchiq_init_state Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice Nicolas Saenz Julienne
2018-10-28 20:45   ` Stefan Wahren
2018-11-06 15:41     ` Nicolas Saenz Julienne
2018-11-06 16:06       ` Stefan Wahren
2018-11-06 18:28         ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 10/18] staging: vchiq_core: don't add a wmb() before remote_event_signal() Nicolas Saenz Julienne
2018-10-26 13:48 ` Nicolas Saenz Julienne [this message]
2018-10-28 21:00   ` [PATCH RFC 11/18] staging: vchiq_arm: use completions instead of semaphores Stefan Wahren
2018-10-26 13:48 ` [PATCH RFC 12/18] staging: vchiq_util: " Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 13/18] staging: vchiq_core: " Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 14/18] staging: vchiq_util: get rid of unneeded memory barriers Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 15/18] stagning: vchiq_core: fix logic redundancy in parse_open Nicolas Saenz Julienne
2018-10-28 20:58   ` Stefan Wahren
2018-10-26 13:48 ` [PATCH RFC 16/18] staging: vchiq_arm: rework probe and init functions Nicolas Saenz Julienne
2018-10-28 21:02   ` Stefan Wahren
2018-10-26 13:48 ` [PATCH RFC 17/18] staging: vchiq_arm: fix open/release cdev functions Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 18/18] staging: vchiq: add more tasks to the TODO list Nicolas Saenz Julienne
2018-10-28 21:11   ` Stefan Wahren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181026134813.7775-12-nsaenzjulienne@suse.de \
    --to=nsaenzjulienne@suse.de \
    --cc=dave.stevenson@raspberrypi.org \
    --cc=devel@driverdev.osuosl.org \
    --cc=eric@anholt.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=stefan.wahren@i2se.com \
    --subject='Re: [PATCH RFC 11/18] staging: vchiq_arm: use completions instead of semaphores' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.