Re: [PATCH 3/4] pipe: mark blocking pipe read and FIFO open sleeps as freezable

[Jan Kara]

On Wed 27-05-26 14:49:11, Dai Junbing wrote:
> Tasks blocked in pipe read or FIFO open may be woken during suspend and
> resume due to freezer state transitions. This can cause avoidable
> activity in the suspend/resume path and add unnecessary overhead.
> 
> Mark these sleeps as freezable so they are not unnecessarily woken by
> the freezer.
> 
> Signed-off-by: Dai Junbing <daijunbing@xxxxxxxx>

So, you don't mention here the reason why not all interruptible sleeps can
be freezable - and that is because you have to make sure we aren't holding
any resources that shouldn't be held when freezing a task. Typically these
are sleeping locks. Freezing task holding a mutex or semaphore or similar
lock is nasty and would need *much* stronger justification than "save a
pointless wakeup on hibernation". 

Making sure we don't hold any lock is rather non-trivial in some cases. For
example in your case here, anon_pipe_read() is used in .read_iter method so
you should make sure all places calling .read_iter don't hold some lock. If
nothing else, I'm missing this analysis in the changelog and I actually
strongly suspect it is not true in some corner cases (like when playing
with splice(2), some files in /proc, or similar).

								Honza


> diff --git a/fs/pipe.c b/fs/pipe.c
> index 9841648c9cf3..594726a7e542 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -385,7 +385,7 @@ anon_pipe_read(struct kiocb *iocb, struct iov_iter *to)
>  		 * since we've done any required wakeups and there's no need
>  		 * to mark anything accessed. And we've dropped the lock.
>  		 */
> -		if (wait_event_interruptible_exclusive(pipe->rd_wait, pipe_readable(pipe)) < 0)
> +		if (wait_event_freezable_exclusive(pipe->rd_wait, pipe_readable(pipe)) < 0)
>  			return -ERESTARTSYS;
>  
>  		wake_next_reader = true;
> @@ -1102,7 +1102,7 @@ static int wait_for_partner(struct pipe_inode_info *pipe, unsigned int *cnt)
>  	int cur = *cnt;
>  
>  	while (cur == *cnt) {
> -		prepare_to_wait(&pipe->rd_wait, &rdwait, TASK_INTERRUPTIBLE);
> +		prepare_to_wait(&pipe->rd_wait, &rdwait, TASK_INTERRUPTIBLE|TASK_FREEZABLE);
>  		pipe_unlock(pipe);
>  		schedule();
>  		finish_wait(&pipe->rd_wait, &rdwait);
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR