Re: [PATCH v4] fs/pipe: unify the page pools into a single per-pipe pool

[Oleg Nesterov <oleg@redhat.com>]

On 07/10, Breno Leitao wrote:
>
> Pipes keep two separate page caches:
>  a) The per-pipe, lock-protected tmp_page[2]
>  b) An on-stack anon_pipe_prealloc burst pool of up to eight pages
>     filled before the lock
>
> Converge them into a single per-pipe pool (struct anon_pipe_prealloc
> embedded in pipe_inode_info) with the same budget as before: up to
> PIPE_PREALLOC_MAX (8) pages, trimmed back to PIPE_PREALLOC_KEEP (2)
> after each operation. tmp_page[2] is removed.
>
> Pages are still allocated and freed outside pipe->mutex; only the
> assignment into the pool is done under it.
>
> anon_pipe_prefill_and_lock() tops the pool up to the write's page count
> -- and returns with pipe->mutex held, so a write acquires the lock only
> once.
>
> anon_pipe_trim_and_unlock() trims the pool under that same lock before
> dropping it, then frees the excess.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Reviewed-by: Mateusz Guzik <mjguzik@gmail.com>

I personally like this change. To me it really makes this logic more
simple/clear.

Reviewed-by: Oleg Nesterov <oleg@redhat.com>


One comment below...

> +static void anon_pipe_prefill_and_lock(struct pipe_inode_info *pipe, size_t total_len)
>  {
> -	unsigned int want, i;
> -	struct page *page;
> -
> -	prealloc->count = 0;
> -	if (total_len <= PAGE_SIZE)
> -		return;
> +	struct page *pages[PIPE_PREALLOC_MAX];
> +	unsigned int want, have, need, n = 0;
>
>  	want = min_t(unsigned int, DIV_ROUND_UP(total_len, PAGE_SIZE),
>  		     PIPE_PREALLOC_MAX);
> +	/* Unlocked read; the pool is refilled under the lock below. */
> +	have = min_t(unsigned int, READ_ONCE(pipe->prealloc.count), want);

And READ_ONCE() is enough correctness wise.

But AFAIK it is not enough to make KCSAN happy. anon_pipe_prealloc_pop/push
which modify ->count under pipe->mutex need WRITE_ONCE() to please KCSAN.

Or anon_pipe_prefill_and_lock() can use data_race(READ_ONCE()).

Oleg.