Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write

[Andy Shevchenko]

Don't you think the Cc list is a bit overloaded?

On Wed, Dec 25, 2024 at 05:42:02PM +0800, WangYuli wrote:
> When a user calls the read/write system call and passes a pipe
> descriptor, the pipe_read/pipe_write functions are invoked:
> 
> 1. pipe_read():
>   1). Checks if the pipe is valid and if there is any data in the
> pipe buffer.
>   2). Waits for data:
>     *If there is no data in the pipe and the write end is still open,
> the current process enters a sleep state (wait_event()) until data
> is written.
>     *If the write end is closed, return 0.
>   3). Reads data:
>     *Wakes up the process and copies data from the pipe's memory
> buffer to user space.
>     *When the buffer is full, the writing process will go to sleep,
> waiting for the pipe state to change to be awakened (using the
> wake_up_interruptible_sync_poll() mechanism). Once data is read
> from the buffer, the writing process can continue writing, and the
> reading process can continue reading new data.
>   4). Returns the number of bytes read upon successful read.
> 
> 2. pipe_write():
>   1). Checks if the pipe is valid and if there is any available
> space in the pipe buffer.
>   2). Waits for buffer space:
>     *If the pipe buffer is full and the reading process has not
> read any data, pipe_write() may put the current process to sleep
> until there is space in the buffer.
>     *If the read end of the pipe is closed (no process is waiting
> to read), an error code -EPIPE is returned, and a SIGPIPE signal may
> be sent to the process.
>   3). Writes data:
>     *If there is enough space in the pipe buffer, pipe_write() copies
> data from the user space buffer to the kernel buffer of the pipe
> (using copy_from_user()).
>     *If the amount of data the user requests to write is larger than
> the available space in the buffer, multiple writes may be required,
> or the process may wait for new space to be freed.
>   4). Wakes up waiting reading processes:
>     *After the data is successfully written, pipe_write() wakes up
> any processes that may be waiting to read data (using the
> wake_up_interruptible_sync_poll() mechanism).
>   5). Returns the number of bytes successfully written.
> 
> Check if there are any waiting processes in the process wait queue
> by introducing wq_has_sleeper() when waking up processes for pipe
> read/write operations.
> 
> If no processes are waiting, there's no need to execute
> wake_up_interruptible_sync_poll(), thus avoiding unnecessary wake-ups.
> 
> Unnecessary wake-ups can lead to context switches, where a process
> is woken up to handle I/O events even when there is no immediate
> need.
> 
> Only wake up processes when there are actually waiting processes to
> reduce context switches and system overhead by checking
> with wq_has_sleeper().
> 
> Additionally, by reducing unnecessary synchronization and wake-up
> operations, wq_has_sleeper() can decrease system resource waste and
> lock contention, improving overall system performance.
> 
> For pipe read/write operations, this eliminates ineffective scheduling
> and enhances concurrency.
> 
> It's important to note that enabling this option means invoking
> wq_has_sleeper() to check for sleeping processes in the wait queue
> for every read or write operation.
> 
> While this is a lightweight operation, it still incurs some overhead.
> 
> In low-load or single-task scenarios, this overhead may not yield
> significant benefits and could even introduce minor performance
> degradation.
> 
> UnixBench Pipe benchmark results on Zhaoxin KX-U6780A processor:
> 
> With the option disabled: Single-core: 841.8, Multi-core (8): 4621.6
> With the option enabled:  Single-core: 877.8, Multi-core (8): 4854.7
> 
> Single-core performance improved by 4.1%, multi-core performance
> improved by 4.8%.

...

> +config PIPE_SKIP_SLEEPER
> +	bool "Skip sleeping processes during pipe read/write"
> +	default n

'n' is the default 'default', no need to have this line.

> +	help
> +	  This option introduces a check whether the sleep queue will
> +	  be awakened during pipe read/write.
> +
> +	  It often leads to a performance improvement. However, in
> +	  low-load or single-task scenarios, it may introduce minor
> +	  performance overhead.

> +	  If unsure, say N.

Illogical, it's already N as you stated by putting a redundant line, but after
removing that line it will make sense.

...

> +static inline bool

Have you build this with Clang and `make W=1 ...`?

> +pipe_check_wq_has_sleeper(struct wait_queue_head *wq_head)
> +{
> +	if (IS_ENABLED(CONFIG_PIPE_SKIP_SLEEPER))
> +		return wq_has_sleeper(wq_head);
> +	else

Redundant.

> +		return true;

	if (!foo)
		return true;

	return bar(...);

> +}

-- 
With Best Regards,
Andy Shevchenko