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

Reply via email to