| Message ID | ba1f5a30be45eec6cf73cfdbf4b4e1679a03cef8.1710343154.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | small rings clean up | expand |
On 3/13/24 20:50, Jens Axboe wrote: > On 3/13/24 9:52 AM, Pavel Begunkov wrote: >> io_mem_alloc() returns a pointer on success and a pointer-encoded error >> otherwise. However, it can only fail with -ENOMEM, just return NULL on >> failure. PTR_ERR is usually pretty error prone. > > I take that back, this is buggy - the io_rings_map() and friends return > an error pointer. So better to keep it consistent. Dropped this one. Oh crap, you're right. Did something trigger it? Because tests are suspiciously silent, I'd assume it's not really tested, e.g. passing misaligned uptr
On 3/13/24 9:52 AM, Pavel Begunkov wrote: > io_mem_alloc() returns a pointer on success and a pointer-encoded error > otherwise. However, it can only fail with -ENOMEM, just return NULL on > failure. PTR_ERR is usually pretty error prone. I take that back, this is buggy - the io_rings_map() and friends return an error pointer. So better to keep it consistent. Dropped this one.
On 3/13/24 2:43 PM, Pavel Begunkov wrote: > On 3/13/24 20:50, Jens Axboe wrote: >> On 3/13/24 9:52 AM, Pavel Begunkov wrote: >>> io_mem_alloc() returns a pointer on success and a pointer-encoded error >>> otherwise. However, it can only fail with -ENOMEM, just return NULL on >>> failure. PTR_ERR is usually pretty error prone. >> >> I take that back, this is buggy - the io_rings_map() and friends return >> an error pointer. So better to keep it consistent. Dropped this one. > Oh crap, you're right. Did something trigger it? Because tests > are suspiciously silent, I'd assume it's not really tested, > e.g. passing misaligned uptr I was doing a cleanup on top, and noticed it while doing that. None of the tests triggered it.
Pavel Begunkov <asml.silence@gmail.com> writes: > io_mem_alloc() returns a pointer on success and a pointer-encoded error > otherwise. However, it can only fail with -ENOMEM, just return NULL on > failure. PTR_ERR is usually pretty error prone. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > io_uring/io_uring.c | 14 +++++--------- > io_uring/kbuf.c | 4 ++-- > 2 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > index e7d7a456b489..1d0eac0cc8aa 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c > @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx) > void *io_mem_alloc(size_t size) > { > gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP; > - void *ret; > > - ret = (void *) __get_free_pages(gfp, get_order(size)); > - if (ret) > - return ret; > - return ERR_PTR(-ENOMEM); > + return (void *) __get_free_pages(gfp, get_order(size)); > } > > static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries, > @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx, > else > rings = io_rings_map(ctx, p->cq_off.user_addr, size); > > - if (IS_ERR(rings)) > - return PTR_ERR(rings); > + if (!rings) > + return -ENOMEM; > Sorry, I started reviewing this, got excited about the error path quick fix, and didn't finish the review before it got it. I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP case, because io_rings_map returns ERR_PTR, and not NULL. In addition, io_rings_map might fail for multiple reasons, and we want to propagate the different error codes up here.
On 3/13/24 4:32 PM, Gabriel Krisman Bertazi wrote: > Pavel Begunkov <asml.silence@gmail.com> writes: > >> io_mem_alloc() returns a pointer on success and a pointer-encoded error >> otherwise. However, it can only fail with -ENOMEM, just return NULL on >> failure. PTR_ERR is usually pretty error prone. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> io_uring/io_uring.c | 14 +++++--------- >> io_uring/kbuf.c | 4 ++-- >> 2 files changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >> index e7d7a456b489..1d0eac0cc8aa 100644 >> --- a/io_uring/io_uring.c >> +++ b/io_uring/io_uring.c >> @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx) >> void *io_mem_alloc(size_t size) >> { >> gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP; >> - void *ret; >> >> - ret = (void *) __get_free_pages(gfp, get_order(size)); >> - if (ret) >> - return ret; >> - return ERR_PTR(-ENOMEM); >> + return (void *) __get_free_pages(gfp, get_order(size)); >> } >> >> static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries, >> @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx, >> else >> rings = io_rings_map(ctx, p->cq_off.user_addr, size); >> >> - if (IS_ERR(rings)) >> - return PTR_ERR(rings); >> + if (!rings) >> + return -ENOMEM; >> > > Sorry, I started reviewing this, got excited about the error path quick > fix, and didn't finish the review before it got it. > > I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP > case, because io_rings_map returns ERR_PTR, and not NULL. In addition, > io_rings_map might fail for multiple reasons, and we want to propagate > the different error codes up here. Yeah, see my reply from some hours ago. I dropped it back then.
Jens Axboe <axboe@kernel.dk> writes: > On 3/13/24 4:32 PM, Gabriel Krisman Bertazi wrote: >> Pavel Begunkov <asml.silence@gmail.com> writes: >> >>> io_mem_alloc() returns a pointer on success and a pointer-encoded error >>> otherwise. However, it can only fail with -ENOMEM, just return NULL on >>> failure. PTR_ERR is usually pretty error prone. >>> >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> io_uring/io_uring.c | 14 +++++--------- >>> io_uring/kbuf.c | 4 ++-- >>> 2 files changed, 7 insertions(+), 11 deletions(-) >>> >>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >>> index e7d7a456b489..1d0eac0cc8aa 100644 >>> --- a/io_uring/io_uring.c >>> +++ b/io_uring/io_uring.c >>> @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx) >>> void *io_mem_alloc(size_t size) >>> { >>> gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP; >>> - void *ret; >>> >>> - ret = (void *) __get_free_pages(gfp, get_order(size)); >>> - if (ret) >>> - return ret; >>> - return ERR_PTR(-ENOMEM); >>> + return (void *) __get_free_pages(gfp, get_order(size)); >>> } >>> >>> static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries, >>> @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx, >>> else >>> rings = io_rings_map(ctx, p->cq_off.user_addr, size); >>> >>> - if (IS_ERR(rings)) >>> - return PTR_ERR(rings); >>> + if (!rings) >>> + return -ENOMEM; >>> >> >> Sorry, I started reviewing this, got excited about the error path quick >> fix, and didn't finish the review before it got it. >> >> I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP >> case, because io_rings_map returns ERR_PTR, and not NULL. In addition, >> io_rings_map might fail for multiple reasons, and we want to propagate >> the different error codes up here. > > Yeah, see my reply from some hours ago. I dropped it back then. ah, thanks. I've configured lei to fetch the io_uring list every few hours. This ended up fetching part of the thread at first, and I only saw it dropped in the next fetch, after I sent the email. sorry for the noise.
On 3/13/24 5:43 PM, Gabriel Krisman Bertazi wrote: > Jens Axboe <axboe@kernel.dk> writes: > >> On 3/13/24 4:32 PM, Gabriel Krisman Bertazi wrote: >>> Pavel Begunkov <asml.silence@gmail.com> writes: >>> >>>> io_mem_alloc() returns a pointer on success and a pointer-encoded error >>>> otherwise. However, it can only fail with -ENOMEM, just return NULL on >>>> failure. PTR_ERR is usually pretty error prone. >>>> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> io_uring/io_uring.c | 14 +++++--------- >>>> io_uring/kbuf.c | 4 ++-- >>>> 2 files changed, 7 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >>>> index e7d7a456b489..1d0eac0cc8aa 100644 >>>> --- a/io_uring/io_uring.c >>>> +++ b/io_uring/io_uring.c >>>> @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx) >>>> void *io_mem_alloc(size_t size) >>>> { >>>> gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP; >>>> - void *ret; >>>> >>>> - ret = (void *) __get_free_pages(gfp, get_order(size)); >>>> - if (ret) >>>> - return ret; >>>> - return ERR_PTR(-ENOMEM); >>>> + return (void *) __get_free_pages(gfp, get_order(size)); >>>> } >>>> >>>> static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries, >>>> @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx, >>>> else >>>> rings = io_rings_map(ctx, p->cq_off.user_addr, size); >>>> >>>> - if (IS_ERR(rings)) >>>> - return PTR_ERR(rings); >>>> + if (!rings) >>>> + return -ENOMEM; >>>> >>> >>> Sorry, I started reviewing this, got excited about the error path quick >>> fix, and didn't finish the review before it got it. >>> >>> I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP >>> case, because io_rings_map returns ERR_PTR, and not NULL. In addition, >>> io_rings_map might fail for multiple reasons, and we want to propagate >>> the different error codes up here. >> >> Yeah, see my reply from some hours ago. I dropped it back then. > > ah, thanks. I've configured lei to fetch the io_uring list every few > hours. This ended up fetching part of the thread at first, and I only saw > it dropped in the next fetch, after I sent the email. sorry for the noise. Oh it's fine, I'd rather have one extra review than none at all :-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index e7d7a456b489..1d0eac0cc8aa 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx) void *io_mem_alloc(size_t size) { gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP; - void *ret; - ret = (void *) __get_free_pages(gfp, get_order(size)); - if (ret) - return ret; - return ERR_PTR(-ENOMEM); + return (void *) __get_free_pages(gfp, get_order(size)); } static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries, @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx, else rings = io_rings_map(ctx, p->cq_off.user_addr, size); - if (IS_ERR(rings)) - return PTR_ERR(rings); + if (!rings) + return -ENOMEM; ctx->rings = rings; if (!(ctx->flags & IORING_SETUP_NO_SQARRAY)) @@ -3787,9 +3783,9 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx, else ptr = io_sqes_map(ctx, p->sq_off.user_addr, size); - if (IS_ERR(ptr)) { + if (!ptr) { io_rings_free(ctx); - return PTR_ERR(ptr); + return -ENOMEM; } ctx->sq_sqes = ptr; diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c index 9be42bff936b..0677eae92e79 100644 --- a/io_uring/kbuf.c +++ b/io_uring/kbuf.c @@ -627,8 +627,8 @@ static int io_alloc_pbuf_ring(struct io_ring_ctx *ctx, ibf = io_lookup_buf_free_entry(ctx, ring_size); if (!ibf) { ptr = io_mem_alloc(ring_size); - if (IS_ERR(ptr)) - return PTR_ERR(ptr); + if (!ptr) + return -ENOMEM; /* Allocate and store deferred free entry */ ibf = kmalloc(sizeof(*ibf), GFP_KERNEL_ACCOUNT);
io_mem_alloc() returns a pointer on success and a pointer-encoded error otherwise. However, it can only fail with -ENOMEM, just return NULL on failure. PTR_ERR is usually pretty error prone. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- io_uring/io_uring.c | 14 +++++--------- io_uring/kbuf.c | 4 ++-- 2 files changed, 7 insertions(+), 11 deletions(-)