[PATCH] ipc/sem.c: Simplify sem_otime handling.

[Manfred Spraul]

The sem_otime handling tries to minimize the number of calls to
get_seconds().
This generates some complexity (i.e.: pass around if any operation
completed) and introduced one bug (See previous commit to ipc/sem.c).

Therefore: Remove the whole logic - this removes 25 lines.

Disadvantages:
- One line of code duplication in exit_sem():
  The function must now update sem_otime, it can't rely on
  do_smart_update_queue() to perform that task.
- Two get_seconds() calls instead of one call for the common
  case of increasing a semaphore and waking up a sleeping task.

TODO:
1) How fast is get_seconds()?
      Is the optimization worth the effort?
2) Test it.

---
 ipc/sem.c | 61 ++++++++++++++++++-------------------------------------------
 1 file changed, 18 insertions(+), 43 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 8e01e76..5e5d7b1 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -713,15 +713,13 @@ static int check_restart(struct sem_array *sma, struct sem_queue *q)
  * semaphore.
  * The tasks that must be woken up are added to @pt. The return code
  * is stored in q->pid.
- * The function returns 1 if at least one operation was completed successfully.
  */
-static int wake_const_ops(struct sem_array *sma, int semnum,
+static void wake_const_ops(struct sem_array *sma, int semnum,
 				struct list_head *pt)
 {
 	struct sem_queue *q;
 	struct list_head *walk;
 	struct list_head *pending_list;
-	int semop_completed = 0;
 
 	if (semnum == -1)
 		pending_list = &sma->pending_const;
@@ -742,13 +740,9 @@ static int wake_const_ops(struct sem_array *sma, int semnum,
 			/* operation completed, remove from queue & wakeup */
 
 			unlink_queue(sma, q);
-
 			wake_up_sem_queue_prepare(pt, q, error);
-			if (error == 0)
-				semop_completed = 1;
 		}
 	}
-	return semop_completed;
 }
 
 /**
@@ -761,13 +755,11 @@ static int wake_const_ops(struct sem_array *sma, int semnum,
  * do_smart_wakeup_zero() checks all required queue for wait-for-zero
  * operations, based on the actual changes that were performed on the
  * semaphore array.
- * The function returns 1 if at least one operation was completed successfully.
  */
-static int do_smart_wakeup_zero(struct sem_array *sma, struct sembuf *sops,
+static void do_smart_wakeup_zero(struct sem_array *sma, struct sembuf *sops,
 					int nsops, struct list_head *pt)
 {
 	int i;
-	int semop_completed = 0;
 	int got_zero = 0;
 
 	/* first: the per-semaphore queues, if known */
@@ -777,7 +769,7 @@ static int do_smart_wakeup_zero(struct sem_array *sma, struct sembuf *sops,
 
 			if (sma->sem_base[num].semval == 0) {
 				got_zero = 1;
-				semop_completed |= wake_const_ops(sma, num, pt);
+				wake_const_ops(sma, num, pt);
 			}
 		}
 	} else {
@@ -788,7 +780,7 @@ static int do_smart_wakeup_zero(struct sem_array *sma, struct sembuf *sops,
 		for (i = 0; i < sma->sem_nsems; i++) {
 			if (sma->sem_base[i].semval == 0) {
 				got_zero = 1;
-				semop_completed |= wake_const_ops(sma, i, pt);
+				wake_const_ops(sma, i, pt);
 			}
 		}
 	}
@@ -797,9 +789,7 @@ static int do_smart_wakeup_zero(struct sem_array *sma, struct sembuf *sops,
 	 * then check the global queue, too.
 	 */
 	if (got_zero)
-		semop_completed |= wake_const_ops(sma, -1, pt);
-
-	return semop_completed;
+		wake_const_ops(sma, -1, pt);
 }
 
 
@@ -816,15 +806,12 @@ static int do_smart_wakeup_zero(struct sem_array *sma, struct sembuf *sops,
  * The tasks that must be woken up are added to @pt. The return code
  * is stored in q->pid.
  * The function internally checks if const operations can now succeed.
- *
- * The function return 1 if at least one semop was completed successfully.
  */
-static int update_queue(struct sem_array *sma, int semnum, struct list_head *pt)
+static void update_queue(struct sem_array *sma, int semnum, struct list_head *pt)
 {
 	struct sem_queue *q;
 	struct list_head *walk;
 	struct list_head *pending_list;
-	int semop_completed = 0;
 
 	if (semnum == -1)
 		pending_list = &sma->pending_alter;
@@ -861,7 +848,6 @@ again:
 		if (error) {
 			restart = 0;
 		} else {
-			semop_completed = 1;
 			do_smart_wakeup_zero(sma, q->sops, q->nsops, pt);
 			restart = check_restart(sma, q);
 		}
@@ -870,15 +856,13 @@ again:
 		if (restart)
 			goto again;
 	}
-	return semop_completed;
 }
 
 /**
- * do_smart_update(sma, sops, nsops, otime, pt) - optimized update_queue
+ * do_smart_update(sma, sops, nsops, pt) - optimized update_queue
  * @sma: semaphore array
  * @sops: operations that were performed
  * @nsops: number of operations
- * @otime: force setting otime
  * @pt: list head of the tasks that must be woken up.
  *
  * do_smart_update() does the required calls to update_queue and wakeup_zero,
@@ -888,15 +872,15 @@ again:
  * It is safe to perform this call after dropping all locks.
  */
 static void do_smart_update(struct sem_array *sma, struct sembuf *sops, int nsops,
-			int otime, struct list_head *pt)
+			struct list_head *pt)
 {
 	int i;
 
-	otime |= do_smart_wakeup_zero(sma, sops, nsops, pt);
+	do_smart_wakeup_zero(sma, sops, nsops, pt);
 
 	if (!list_empty(&sma->pending_alter)) {
 		/* semaphore array uses the global queue - just process it. */
-		otime |= update_queue(sma, -1, pt);
+		update_queue(sma, -1, pt);
 	} else {
 		if (!sops) {
 			/*
@@ -904,7 +888,7 @@ static void do_smart_update(struct sem_array *sma, struct sembuf *sops, int nsop
 			 * known. Check all.
 			 */
 			for (i = 0; i < sma->sem_nsems; i++)
-				otime |= update_queue(sma, i, pt);
+				update_queue(sma, i, pt);
 		} else {
 			/*
 			 * Check the semaphores that were increased:
@@ -916,21 +900,11 @@ static void do_smart_update(struct sem_array *sma, struct sembuf *sops, int nsop
 			 *   value will be too small, too.
 			 */
 			for (i = 0; i < nsops; i++) {
-				if (sops[i].sem_op > 0) {
-					otime |= update_queue(sma,
-							sops[i].sem_num, pt);
-				}
+				if (sops[i].sem_op > 0)
+					update_queue(sma, sops[i].sem_num, pt);
 			}
 		}
 	}
-	if (otime) {
-		if (sops == NULL) {
-			sma->sem_base[0].sem_otime = get_seconds();
-		} else {
-			sma->sem_base[sops[0].sem_num].sem_otime =
-								get_seconds();
-		}
-	}
 }
 
 
@@ -1238,7 +1212,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
 	curr->sempid = task_tgid_vnr(current);
 	sma->sem_ctime = get_seconds();
 	/* maybe some queued-up processes were waiting for this */
-	do_smart_update(sma, NULL, 0, 0, &tasks);
+	do_smart_update(sma, NULL, 0, &tasks);
 	sem_unlock(sma, -1);
 	rcu_read_unlock();
 	wake_up_sem_queue_do(&tasks);
@@ -1366,7 +1340,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 		}
 		sma->sem_ctime = get_seconds();
 		/* maybe some queued-up processes were waiting for this */
-		do_smart_update(sma, NULL, 0, 0, &tasks);
+		do_smart_update(sma, NULL, 0, &tasks);
 		err = 0;
 		goto out_unlock;
 	}
@@ -1798,7 +1772,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 					task_tgid_vnr(current));
 	if (error <= 0) {
 		if (alter && error == 0)
-			do_smart_update(sma, sops, nsops, 1, &tasks);
+			do_smart_update(sma, sops, nsops, &tasks);
 
 		goto out_unlock_free;
 	}
@@ -2043,7 +2017,8 @@ void exit_sem(struct task_struct *tsk)
 		}
 		/* maybe some queued-up processes were waiting for this */
 		INIT_LIST_HEAD(&tasks);
-		do_smart_update(sma, NULL, 0, 1, &tasks);
+		sma->sem_base[0].sem_otime = get_seconds();
+		do_smart_update(sma, NULL, 0, &tasks);
 		sem_unlock(sma, -1);
 		rcu_read_unlock();
 		wake_up_sem_queue_do(&tasks);
-- 
1.8.3.1


--------------050909090502070805080603--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/