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

[Manfred Spraul]

Hi Davidlohr,

On 09/12/2016 01:53 PM, Davidlohr Bueso wrote:
> @@ -1933,22 +1823,32 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
>   	queue.alter = alter;
>   
>   	error = perform_atomic_semop(sma, &queue);
> -	if (error == 0) {
> -		/* If the operation was successful, then do
> +	if (error <= 0) { /* non-blocking path */
> +		WAKE_Q(wake_q);
> +
> +		/*
> +		 * If the operation was successful, then do
>   		 * the required updates.
>   		 */
> -		if (alter)
> -			do_smart_update(sma, sops, nsops, 1, &tasks);
> -		else
> -			set_semotime(sma, sops);
> +		if (error == 0) {
> +			if (alter)
> +				do_smart_update(sma, sops, nsops, 1, &wake_q);
> +			else
> +				set_semotime(sma, sops);
> + <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Why this empty line?
> +		}
> +
> +		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.

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

--
    Manfred