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

[Davidlohr Bueso]

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