Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1485919 > unrolled thread

[PATCH -next v2 0/5] ipc/sem: semop(2) improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH -next v2 0/5] ipc/sem: semop(2) improvements Davidlohr Bueso <dave@stgolabs.net> - 2016-09-18 21:20 +0200
    [PATCH 5/5] ipc/sem: use proper list api for pending_list wakeups Davidlohr Bueso <dave@stgolabs.net> - 2016-09-18 21:20 +0200
    [PATCH 4/5] ipc/sem: explicitly inline check_restart Davidlohr Bueso <dave@stgolabs.net> - 2016-09-18 21:20 +0200
    Re: [PATCH -next v2 0/5] ipc/sem: semop(2) improvements Manfred Spraul <manfred@colorfullife.com> - 2016-09-19 20:50 +0200
      Re: [PATCH -next v2 0/5] ipc/sem: semop(2) improvements Davidlohr Bueso <dave@stgolabs.net> - 2016-09-20 17:10 +0200

#1485919 — [PATCH -next v2 0/5] ipc/sem: semop(2) improvements

Changes from v1 (https://lkml.org/lkml/2016/9/12/266)
- Got rid of the signal_pending check in wakeup fastpath. (patch 2)
- Added read/access once to queue.status (we're obviously concerned about
 lockless access upon unrelated events, even if on the stack).
- Got rid of initializing wake_q and wake_up_q call upon perform_atomic_semop
  error return path. (patch 2)
- Documented ordering between wake_q_add and setting ->status.
- What I did not do was refactor the checks in perfor_atomic_semop[_slow]
  as I could not get a decent/clean way of doing it without adding more
  unnecessary code. If we wanted to do smart semop scans that we received from
  userspace, this would still need to be done under sem_lock for semval values
  obviously. So I've left it as is, where we mainly duplicate the function, but
  I still believe this is the most straightforward way of dealing with this
  situation  (patch 3).
- Replaced using SEMOP_FAST with BITS_PER_LONG, as this is really what we want
  to limit the duplicate scanning.
- More testing.
- Added Manfred's ack (patch 5).

Hi,

Here are a few updates around the semop syscall handling that I noticed while
reviewing Manfred's simple vs complex ops fixes. Changes are on top of -next,
which means that Manfred's pending patches to ipc/sem.c that remove the redundant
barrier(s) would probably have to be rebased.

The patchset has survived the following testscases:
- ltp
- ipcsemtest (https://github.com/manfred-colorfu/ipcsemtest)
- ipcscale (https://github.com/manfred-colorfu/ipcscale)

Details are in each individual patch. Please consider for v4.9.

Thanks!

Davidlohr Bueso (5):
  ipc/sem: do not call wake_sem_queue_do() prematurely
  ipc/sem: rework task wakeups
  ipc/sem: optimize perform_atomic_semop()
  ipc/sem: explicitly inline check_restart
  ipc/sem: use proper list api for pending_list wakeups

 ipc/sem.c | 415 ++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 199 insertions(+), 216 deletions(-)

-- 
2.6.6

[toc] | [next] | [standalone]

#1485921 — [PATCH 5/5] ipc/sem: use proper list api for pending_list wakeups

... saves some LoC and looks cleaner than re-implementing the
calls.

Acked-by: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/sem.c | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 89adba51e85f..e7da9821bf46 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -812,8 +812,7 @@ static inline int check_restart(struct sem_array *sma, struct sem_queue *q)
 static int wake_const_ops(struct sem_array *sma, int semnum,
 			  struct wake_q_head *wake_q)
 {
-	struct sem_queue *q;
-	struct list_head *walk;
+	struct sem_queue *q, *tmp;
 	struct list_head *pending_list;
 	int semop_completed = 0;
 
@@ -822,25 +821,19 @@ static int wake_const_ops(struct sem_array *sma, int semnum,
 	else
 		pending_list = &sma->sem_base[semnum].pending_const;
 
-	walk = pending_list->next;
-	while (walk != pending_list) {
-		int error;
-
-		q = container_of(walk, struct sem_queue, list);
-		walk = walk->next;
-
-		error = perform_atomic_semop(sma, q);
-
-		if (error <= 0) {
-			/* operation completed, remove from queue & wakeup */
+	list_for_each_entry_safe(q, tmp, pending_list, list) {
+		int error = perform_atomic_semop(sma, q);
 
-			unlink_queue(sma, q);
+		if (error > 0)
+			continue;
+		/* operation completed, remove from queue & wakeup */
+		unlink_queue(sma, q);
 
-			wake_up_sem_queue_prepare(q, error, wake_q);
-			if (error == 0)
-				semop_completed = 1;
-		}
+		wake_up_sem_queue_prepare(q, error, wake_q);
+		if (error == 0)
+			semop_completed = 1;
 	}
+
 	return semop_completed;
 }
 
@@ -913,8 +906,7 @@ static int do_smart_wakeup_zero(struct sem_array *sma, struct sembuf *sops,
  */
 static int update_queue(struct sem_array *sma, int semnum, struct wake_q_head *wake_q)
 {
-	struct sem_queue *q;
-	struct list_head *walk;
+	struct sem_queue *q, *tmp;
 	struct list_head *pending_list;
 	int semop_completed = 0;
 
@@ -924,13 +916,9 @@ static int update_queue(struct sem_array *sma, int semnum, struct wake_q_head *w
 		pending_list = &sma->sem_base[semnum].pending_alter;
 
 again:
-	walk = pending_list->next;
-	while (walk != pending_list) {
+	list_for_each_entry_safe(q, tmp, pending_list, list) {
 		int error, restart;
 
-		q = container_of(walk, struct sem_queue, list);
-		walk = walk->next;
-
 		/* If we are scanning the single sop, per-semaphore list of
 		 * one semaphore and that semaphore is 0, then it is not
 		 * necessary to scan further: simple increments
-- 
2.6.6

[toc] | [prev] | [next] | [standalone]

#1485923 — [PATCH 4/5] ipc/sem: explicitly inline check_restart

The compiler already does this, but make it explicit. This helper
is really small and also used in update_queue's main loop, which is
O(N^2) scanning. Inline and avoid the function overhead.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/sem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index e868b5933ff8..89adba51e85f 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -771,7 +771,7 @@ static void unlink_queue(struct sem_array *sma, struct sem_queue *q)
  * modified the array.
  * Note that wait-for-zero operations are handled without restart.
  */
-static int check_restart(struct sem_array *sma, struct sem_queue *q)
+static inline int check_restart(struct sem_array *sma, struct sem_queue *q)
 {
 	/* pending complex alter operations are too difficult to analyse */
 	if (!list_empty(&sma->pending_alter))
-- 
2.6.6

[toc] | [prev] | [next] | [standalone]

#1486762

On 09/18/2016 09:11 PM, Davidlohr Bueso wrote:
> Changes from v1 (https://lkml.org/lkml/2016/9/12/266)
> - Got rid of the signal_pending check in wakeup fastpath. (patch 2)
> - Added read/access once to queue.status (we're obviously concerned about
>   lockless access upon unrelated events, even if on the stack).
> - Got rid of initializing wake_q and wake_up_q call upon perform_atomic_semop
>    error return path. (patch 2)
> - Documented ordering between wake_q_add and setting ->status.
> - What I did not do was refactor the checks in perfor_atomic_semop[_slow]
>    as I could not get a decent/clean way of doing it without adding more
>    unnecessary code. If we wanted to do smart semop scans that we received from
>    userspace, this would still need to be done under sem_lock for semval values
>    obviously. So I've left it as is, where we mainly duplicate the function, but
>    I still believe this is the most straightforward way of dealing with this
>    situation  (patch 3).
> - Replaced using SEMOP_FAST with BITS_PER_LONG, as this is really what we want
>    to limit the duplicate scanning.
> - More testing.
> - Added Manfred's ack (patch 5).
>
> Hi,
>
> Here are a few updates around the semop syscall handling that I noticed while
> reviewing Manfred's simple vs complex ops fixes. Changes are on top of -next,
> which means that Manfred's pending patches to ipc/sem.c that remove the redundant
> barrier(s) would probably have to be rebased.
>
> The patchset has survived the following testscases:
> - ltp
> - ipcsemtest (https://github.com/manfred-colorfu/ipcsemtest)
> - ipcscale (https://github.com/manfred-colorfu/ipcscale)
>
> Details are in each individual patch. Please consider for v4.9.
>
> Thanks!
>
> Davidlohr Bueso (5):
>    ipc/sem: do not call wake_sem_queue_do() prematurely
The only patch that I don't like.
Especially: patch 2 of the series removes the wake_up_q from the 
function epilogue.
So only the code duplication (additional instances of rcu_read_unlock()) 
remains, I don't see any advantages.

>    ipc/sem: rework task wakeups
Acked
>    ipc/sem: optimize perform_atomic_semop()
I'm still thinking about it.
Code duplication is evil, but perhaps it is the best solution.

What I don't like is the hardcoded "< BITS_PER_LONG".
At least:
- (1 << sop->sem_num)
+ (1 << (sop->sem_num%BITS_PER_LONG))
>    ipc/sem: explicitly inline check_restart
Do we really need that? Isn't that the compiler's task?
Especially since the compiler is already doing it correctly.
>    ipc/sem: use proper list api for pending_list wakeups
Acked
>   ipc/sem.c | 415 ++++++++++++++++++++++++++++++--------------------------------
>   1 file changed, 199 insertions(+), 216 deletions(-)
>
--

     Manfred

[toc] | [prev] | [next] | [standalone]

#1487421

On Mon, 19 Sep 2016, Manfred Spraul wrote:
>On 09/18/2016 09:11 PM, Davidlohr Bueso wrote:

>>Davidlohr Bueso (5):
>>   ipc/sem: do not call wake_sem_queue_do() prematurely
>The only patch that I don't like.
>Especially: patch 2 of the series removes the wake_up_q from the 
>function epilogue.
>So only the code duplication (additional instances of 
>rcu_read_unlock()) remains, I don't see any advantages.
>
>>   ipc/sem: rework task wakeups
>Acked

Thanks.

>>   ipc/sem: optimize perform_atomic_semop()
>I'm still thinking about it.
>Code duplication is evil, but perhaps it is the best solution.
>
>What I don't like is the hardcoded "< BITS_PER_LONG".
>At least:
>- (1 << sop->sem_num)
>+ (1 << (sop->sem_num%BITS_PER_LONG))

Yeah, I'll send v3 for that.

>>   ipc/sem: explicitly inline check_restart
>Do we really need that? Isn't that the compiler's task?
>Especially since the compiler is already doing it correctly.

Yes, I mentioned in the changelog that the compiler does it and this is
merely explicit. That said I see no harm in it, I guess whatever akpm says.

>>   ipc/sem: use proper list api for pending_list wakeups
>Acked

Thanks,
Davidlohr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web