| Message ID | 731ecc625e6e67900ebe8c821b3d3647850e0bea.1692119257.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | caching and SQ/CQ optimisations | expand |
On 8/15/23 11:31 AM, Pavel Begunkov wrote: > io_kiocb::cqe stores the completion info which we'll memcpy to > userspace, and we rely on callbacks and other later steps to populate > it with right values. We have never had problems with that, but it would > still be safer to zero it on allocation. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > io_uring/io_uring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > index e189158ebbdd..4d27655be3a6 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c > @@ -1056,7 +1056,7 @@ static void io_preinit_req(struct io_kiocb *req, struct io_ring_ctx *ctx) > req->link = NULL; > req->async_data = NULL; > /* not necessary, but safer to zero */ > - req->cqe.res = 0; > + memset(&req->cqe, 0, sizeof(req->cqe)); > } > > static void io_flush_cached_locked_reqs(struct io_ring_ctx *ctx, I think this is a good idea, but I wonder if we should open-clear it instead. I've had cases in the past where that's more efficient than calling memset.
On 8/19/23 16:03, Jens Axboe wrote: > On 8/15/23 11:31 AM, Pavel Begunkov wrote: >> io_kiocb::cqe stores the completion info which we'll memcpy to >> userspace, and we rely on callbacks and other later steps to populate >> it with right values. We have never had problems with that, but it would >> still be safer to zero it on allocation. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> io_uring/io_uring.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >> index e189158ebbdd..4d27655be3a6 100644 >> --- a/io_uring/io_uring.c >> +++ b/io_uring/io_uring.c >> @@ -1056,7 +1056,7 @@ static void io_preinit_req(struct io_kiocb *req, struct io_ring_ctx *ctx) >> req->link = NULL; >> req->async_data = NULL; >> /* not necessary, but safer to zero */ >> - req->cqe.res = 0; >> + memset(&req->cqe, 0, sizeof(req->cqe)); >> } >> >> static void io_flush_cached_locked_reqs(struct io_ring_ctx *ctx, > > I think this is a good idea, but I wonder if we should open-clear it > instead. I've had cases in the past where that's more efficient than > calling memset. I don't think it ever happens for 16 byte memcpy, and in either case it's a cache refill, quite a slow path. I believe memcpy is better here.
On 8/24/23 10:28 AM, Pavel Begunkov wrote: > On 8/19/23 16:03, Jens Axboe wrote: >> On 8/15/23 11:31 AM, Pavel Begunkov wrote: >>> io_kiocb::cqe stores the completion info which we'll memcpy to >>> userspace, and we rely on callbacks and other later steps to populate >>> it with right values. We have never had problems with that, but it would >>> still be safer to zero it on allocation. >>> >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> io_uring/io_uring.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >>> index e189158ebbdd..4d27655be3a6 100644 >>> --- a/io_uring/io_uring.c >>> +++ b/io_uring/io_uring.c >>> @@ -1056,7 +1056,7 @@ static void io_preinit_req(struct io_kiocb *req, struct io_ring_ctx *ctx) >>> req->link = NULL; >>> req->async_data = NULL; >>> /* not necessary, but safer to zero */ >>> - req->cqe.res = 0; >>> + memset(&req->cqe, 0, sizeof(req->cqe)); >>> } >>> static void io_flush_cached_locked_reqs(struct io_ring_ctx *ctx, >> >> I think this is a good idea, but I wonder if we should open-clear it >> instead. I've had cases in the past where that's more efficient than >> calling memset. > > I don't think it ever happens for 16 byte memcpy, and in either > case it's a cache refill, quite a slow path. I believe memcpy is > better here. Yeah I think it's fine as-is - just checked here and either approach yields the same.
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index e189158ebbdd..4d27655be3a6 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -1056,7 +1056,7 @@ static void io_preinit_req(struct io_kiocb *req, struct io_ring_ctx *ctx) req->link = NULL; req->async_data = NULL; /* not necessary, but safer to zero */ - req->cqe.res = 0; + memset(&req->cqe, 0, sizeof(req->cqe)); } static void io_flush_cached_locked_reqs(struct io_ring_ctx *ctx,
io_kiocb::cqe stores the completion info which we'll memcpy to userspace, and we rely on callbacks and other later steps to populate it with right values. We have never had problems with that, but it would still be safer to zero it on allocation. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- io_uring/io_uring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)