Messages in this thread | | | Date | Sun, 18 Sep 2016 11:26:46 -0700 | From | Davidlohr Bueso <> | Subject | Re: [PATCH 2/5] ipc/sem: rework task wakeups |
| |
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
| |