| Message ID | 20241106122659.730712-4-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: support group buffer & ublk zc | expand |
On 11/6/24 5:26 AM, Ming Lei wrote: > `struct io_mapped_buf` will be extended to cover kernel buffer which > may be in fast IO path, and `struct io_mapped_buf` needs to be per-IO. > > So shrink sizeof(struct io_mapped_buf) by the following ways: > > - folio_shift is < 64, so 6bits are enough to hold it, the remained bits > can be used for the coming kernel buffer > > - define `acct_pages` as 'unsigned int', which is big enough for > accounting pages in the buffer > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > io_uring/rsrc.c | 2 ++ > io_uring/rsrc.h | 6 +++--- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c > index 9b8827c72230..16f5abe03d10 100644 > --- a/io_uring/rsrc.c > +++ b/io_uring/rsrc.c > @@ -685,6 +685,8 @@ static bool io_try_coalesce_buffer(struct page ***pages, int *nr_pages, > return false; > > data->folio_shift = folio_shift(folio); > + WARN_ON_ONCE(data->folio_shift >= 64); Since folio_shift is 6 bits, how can that be try? I think you'd want: WARN_ON_ONCE(folio_shift(folio) >= 64); instead. And agree that acct_pages doesn't need to be an unsigned long.
On Wed, Nov 06, 2024 at 08:09:38AM -0700, Jens Axboe wrote: > On 11/6/24 5:26 AM, Ming Lei wrote: > > `struct io_mapped_buf` will be extended to cover kernel buffer which > > may be in fast IO path, and `struct io_mapped_buf` needs to be per-IO. > > > > So shrink sizeof(struct io_mapped_buf) by the following ways: > > > > - folio_shift is < 64, so 6bits are enough to hold it, the remained bits > > can be used for the coming kernel buffer > > > > - define `acct_pages` as 'unsigned int', which is big enough for > > accounting pages in the buffer > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > io_uring/rsrc.c | 2 ++ > > io_uring/rsrc.h | 6 +++--- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c > > index 9b8827c72230..16f5abe03d10 100644 > > --- a/io_uring/rsrc.c > > +++ b/io_uring/rsrc.c > > @@ -685,6 +685,8 @@ static bool io_try_coalesce_buffer(struct page ***pages, int *nr_pages, > > return false; > > > > data->folio_shift = folio_shift(folio); > > + WARN_ON_ONCE(data->folio_shift >= 64); > > Since folio_shift is 6 bits, how can that be try? > > I think you'd want: > > WARN_ON_ONCE(folio_shift(folio) >= 64); > > instead. imu->folio_shift is 6 bits, and it is only copied from data->folio_shift(char), that is why the warning is added for data->folio_shift. Thanks, Ming
On 11/6/24 6:04 PM, Ming Lei wrote: > On Wed, Nov 06, 2024 at 08:09:38AM -0700, Jens Axboe wrote: >> On 11/6/24 5:26 AM, Ming Lei wrote: >>> `struct io_mapped_buf` will be extended to cover kernel buffer which >>> may be in fast IO path, and `struct io_mapped_buf` needs to be per-IO. >>> >>> So shrink sizeof(struct io_mapped_buf) by the following ways: >>> >>> - folio_shift is < 64, so 6bits are enough to hold it, the remained bits >>> can be used for the coming kernel buffer >>> >>> - define `acct_pages` as 'unsigned int', which is big enough for >>> accounting pages in the buffer >>> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>> --- >>> io_uring/rsrc.c | 2 ++ >>> io_uring/rsrc.h | 6 +++--- >>> 2 files changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c >>> index 9b8827c72230..16f5abe03d10 100644 >>> --- a/io_uring/rsrc.c >>> +++ b/io_uring/rsrc.c >>> @@ -685,6 +685,8 @@ static bool io_try_coalesce_buffer(struct page ***pages, int *nr_pages, >>> return false; >>> >>> data->folio_shift = folio_shift(folio); >>> + WARN_ON_ONCE(data->folio_shift >= 64); >> >> Since folio_shift is 6 bits, how can that be try? >> >> I think you'd want: >> >> WARN_ON_ONCE(folio_shift(folio) >= 64); >> >> instead. > > imu->folio_shift is 6 bits, and it is only copied from data->folio_shift(char), > that is why the warning is added for data->folio_shift. Ah yes that is fine then, for some reason I read that as 'data' being an io_mapped_buffer.
diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 9b8827c72230..16f5abe03d10 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -685,6 +685,8 @@ static bool io_try_coalesce_buffer(struct page ***pages, int *nr_pages, return false; data->folio_shift = folio_shift(folio); + WARN_ON_ONCE(data->folio_shift >= 64); + /* * Check if pages are contiguous inside a folio, and all folios have * the same page count except for the head and tail. diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h index 887699400e29..255ec94ea172 100644 --- a/io_uring/rsrc.h +++ b/io_uring/rsrc.h @@ -30,9 +30,9 @@ struct io_mapped_buf { u64 start; unsigned int len; unsigned int nr_bvecs; - unsigned int folio_shift; refcount_t refs; - unsigned long acct_pages; + unsigned int acct_pages; + unsigned int folio_shift:6; struct bio_vec bvec[] __counted_by(nr_bvecs); }; @@ -41,7 +41,7 @@ struct io_imu_folio_data { unsigned int nr_pages_head; /* For non-head/tail folios, has to be fully included */ unsigned int nr_pages_mid; - unsigned int folio_shift; + unsigned char folio_shift; }; struct io_rsrc_node *io_rsrc_node_alloc(struct io_ring_ctx *ctx, int type);
`struct io_mapped_buf` will be extended to cover kernel buffer which may be in fast IO path, and `struct io_mapped_buf` needs to be per-IO. So shrink sizeof(struct io_mapped_buf) by the following ways: - folio_shift is < 64, so 6bits are enough to hold it, the remained bits can be used for the coming kernel buffer - define `acct_pages` as 'unsigned int', which is big enough for accounting pages in the buffer Signed-off-by: Ming Lei <ming.lei@redhat.com> --- io_uring/rsrc.c | 2 ++ io_uring/rsrc.h | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-)