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

Groups > linux.kernel > #1485901

Re: [PATCH 2/5] ipc/sem: rework task wakeups

Show all headers | View raw

On Sun, 18 Sep 2016, Manfred Spraul wrote:
>>+ <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Why this empty line?

That's my fat fingers, will remove it.

>>+		}
>>+
>>+		sem_unlock(sma, locknum);
>>+		rcu_read_unlock();
>>+		wake_up_q(&wake_q);
>>+
>>+		goto out_free;
>>  	}
>>-	if (error <= 0)
>>-		goto out_unlock_free;
>I don't see the strategy:
>I've used the approach that cleanup is at the end, to reduce 
>duplicated code, even if it means that error codepaths unnecessarily 
>call wakeup for an empty list and that the list is always initialized.
>
>With patch 1 of the series, you start to optimize for that.
>Now this patch reintroduces some wake_up_q calls for error paths.

Well yes, but this is a much more self contained than what we currently have
in that at least perform_atomic_semop() was called. Yes, an error path will
still call wake_up_q unnecessarily, but its pretty obvious what's going on within
that error <= 0 condition. I really don't think this is a big deal. In addition
the general exit path of the function is also slightly cleaned up as a consequence.

>So: What is the aim?
>I would propose to skip patch 1 and leave the wake_up_q at the end.
>
>Or, if we really want to avoid the wakeup calls, then do it entirely.
>Perhaps:
>> if(error == 0) { /* nonblocking codepath 1, with wakeups */
>> [...]
>> }
>> if (error < 0} goto out_unlock_free;
>>
>This would have an advantage, because the WAKE_Q would be initialized 
>only when needed

Sure. Note that we can even get picky with this in semctl calls, but I'm
ok with some unnecessary initialization and wake_up_q paths. Please shout
if you really want me to change them and I can add followup patches, although
I suspect you'll agree.

Thanks,
Davidlohr

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH -next 0/5] ipc/sem: semop(2) improvements Davidlohr Bueso <dave@stgolabs.net> - 2016-09-12 14:00 +0200
  [PATCH 3/5] ipc/sem: optimize perform_atomic_semop() Davidlohr Bueso <dave@stgolabs.net> - 2016-09-12 14:00 +0200
    Re: [PATCH 3/5] ipc/sem: optimize perform_atomic_semop() Manfred Spraul <manfred@colorfullife.com> - 2016-09-12 20:00 +0200
      Re: [PATCH 3/5] ipc/sem: optimize perform_atomic_semop() Davidlohr Bueso <dave@stgolabs.net> - 2016-09-13 10:40 +0200
        Re: [PATCH 3/5] ipc/sem: optimize perform_atomic_semop() Manfred Spraul <manfred@colorfullife.com> - 2016-09-19 06:50 +0200
  [PATCH 2/5] ipc/sem: rework task wakeups Davidlohr Bueso <dave@stgolabs.net> - 2016-09-12 14:00 +0200
    Re: [PATCH 2/5] ipc/sem: rework task wakeups Manfred Spraul <manfred@colorfullife.com> - 2016-09-13 20:10 +0200
      Re: [PATCH 2/5] ipc/sem: rework task wakeups Davidlohr Bueso <dave@stgolabs.net> - 2016-09-14 17:50 +0200
    Re: [PATCH 2/5] ipc/sem: rework task wakeups Manfred Spraul <manfred@colorfullife.com> - 2016-09-18 16:40 +0200
      Re: [PATCH 2/5] ipc/sem: rework task wakeups Davidlohr Bueso <dave@stgolabs.net> - 2016-09-18 20:30 +0200
  [PATCH 4/5] ipc/sem: explicitly inline check_restart Davidlohr Bueso <dave@stgolabs.net> - 2016-09-12 14:00 +0200
  [PATCH 5/5] ipc/sem: use proper list api for pending_list wakeups Davidlohr Bueso <dave@stgolabs.net> - 2016-09-12 14:00 +0200
    Re: [PATCH 5/5] ipc/sem: use proper list api for pending_list wakeups Manfred Spraul <manfred@colorfullife.com> - 2016-09-18 20:00 +0200

csiph-web