Received-SPF: pass (google.com: domain of linux-kernel+bounces-169855-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <71c1f01f-f740-43b0-9962-afcf08cab686@kernel.dk>
Date: Mon, 6 May 2024 06:57:44 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] io_uring/rsrc: Add support for multi-folio buffer
 coalescing
To: Chenliang Li <cliang01.li@samsung.com>, asml.silence@gmail.com
Cc: io-uring@vger.kernel.org, linux-kernel@vger.kernel.org,
 peiwei.li@samsung.com, joshi.k@samsung.com, kundan.kumar@samsung.com,
 gost.dev@samsung.com
References: <CGME20240506075314epcas5p25333b80c8d6a3217d5352a5a7ed89278@epcas5p2.samsung.com>
 <20240506075303.25630-1-cliang01.li@samsung.com>
Content-Language: en-US
From: Jens Axboe <axboe@kernel.dk>
In-Reply-To: <20240506075303.25630-1-cliang01.li@samsung.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 5/6/24 1:53 AM, Chenliang Li wrote:
> Currently fixed buffers consisting of pages in one same folio(huge page)
> can be coalesced into a single bvec entry at registration.
> This patch expands it to support coalescing fixed buffers
> with multiple folios, by:
> 1. Add a helper function and a helper struct to do the coalescing work
> at buffer registration;
> 2. Add the bvec setup procedure of the coalsced path;

coalesced

> 3. store page_mask and page_shift into io_mapped_ubuf for
> later use in io_import_fixed.

Can you add some justification to this commit message? A good commit
message should basically be the WHY of why this commit exists in the
first place. Your commit message just explains what the patch does,
which I can just read the code to see for myself.

As it stands, it's not clear to me or anyone casually reading this
commit message why the change is being done in the first place.

Outside of that, you probably want to split this into two parts - one
that adds the helper for the existing code, then one that modifies it
for your change. We need this to be as simple as possible to review, as
we've had a security issue with page coalescing in this code in the
past.

Minor comments below, will wait with a full review until this is split
to be more easily reviewable.

> +/*
> + * For coalesce to work, a buffer must be one or multiple
> + * folios, all the folios except the first and last one
> + * should be of the same size.
> + */
> +static bool io_sqe_buffer_try_coalesce(struct page **pages,
> +				       unsigned int nr_pages,
> +				       struct io_imu_folio_stats *stats)
> +{
> +	struct folio	*folio = NULL, *first_folio = NULL;
> +	unsigned int	page_cnt;
> +	int		i, j;

Please don't make up your own style, follow the style that's already in
the file to begin with.

> diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h
> index c032ca3436ca..4c655e446150 100644
> --- a/io_uring/rsrc.h
> +++ b/io_uring/rsrc.h
> @@ -47,9 +47,18 @@ struct io_mapped_ubuf {
>  	u64		ubuf_end;
>  	unsigned int	nr_bvecs;
>  	unsigned long	acct_pages;
> +	unsigned int	page_shift;
> +	unsigned long	page_mask;
>  	struct bio_vec	bvec[] __counted_by(nr_bvecs);
>  };

When adding members to a struct, please be cognizant of how it packs.
I'd suggest making the above:

  	u64		ubuf_end;
  	unsigned int	nr_bvecs;
	unsigned int	page_shift;
	unsigned long	page_mask;
  	unsigned long	acct_pages;
	struct bio_vec	bvec[] __counted_by(nr_bvecs);

which should pack much nicer and actually save memory.

-- 
Jens Axboe