| Message ID | 20220613101157.3687-1-dylany@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | io_uring: fixes for provided buffer ring | expand |
On 6/13/22 18:11, Dylan Yudaken wrote: > This fixes two problems in the new provided buffer ring feature. One > is a simple arithmetic bug (I think this came out from a refactor). > The other is due to type differences between head & tail, which causes > it to sometimes reuse an old buffer incorrectly. > > Patch 1&2 fix bugs > Patch 3 limits the size of the ring as it's not > possible to address more entries with 16 bit head/tail Reviewed-by: Hao Xu <howeyxu@tencent.com> > > I will send test cases for liburing shortly. > > One question might be if we should change the type of ring_entries > to uint16_t in struct io_uring_buf_reg? Why not? 5.19 is just rc2 now. So we can assume there is no users using it right now I think?
On Mon, 13 Jun 2022 03:11:54 -0700, Dylan Yudaken wrote: > This fixes two problems in the new provided buffer ring feature. One > is a simple arithmetic bug (I think this came out from a refactor). > The other is due to type differences between head & tail, which causes > it to sometimes reuse an old buffer incorrectly. > > Patch 1&2 fix bugs > Patch 3 limits the size of the ring as it's not > possible to address more entries with 16 bit head/tail > > [...] Applied, thanks! [1/3] io_uring: fix index calculation commit: 97da4a537924d87e2261773f3ac9365abb191fc9 [2/3] io_uring: fix types in provided buffer ring commit: c6e9fa5c0ab811f4bec36a96337f4b1bb77d142c [3/3] io_uring: limit size of provided buffer ring commit: f9437ac0f851cea2374d53594f52fbbefdd977bd Best regards,
On 6/13/22 12:08, Hao Xu wrote: > On 6/13/22 18:11, Dylan Yudaken wrote: >> This fixes two problems in the new provided buffer ring feature. One >> is a simple arithmetic bug (I think this came out from a refactor). >> The other is due to type differences between head & tail, which causes >> it to sometimes reuse an old buffer incorrectly. >> >> Patch 1&2 fix bugs >> Patch 3 limits the size of the ring as it's not >> possible to address more entries with 16 bit head/tail > > Reviewed-by: Hao Xu <howeyxu@tencent.com> > >> >> I will send test cases for liburing shortly. >> >> One question might be if we should change the type of ring_entries >> to uint16_t in struct io_uring_buf_reg? > > Why not? 5.19 is just rc2 now. So we can assume there is no users using > it right now I think? It's fine to change, but might be better if we want to extend it in the future. Do other pbuf bits allow more than 2^16 buffers?
On Mon, 2022-06-13 at 13:59 +0100, Pavel Begunkov wrote: > On 6/13/22 12:08, Hao Xu wrote: > > On 6/13/22 18:11, Dylan Yudaken wrote: > > > This fixes two problems in the new provided buffer ring feature. > > > One > > > is a simple arithmetic bug (I think this came out from a > > > refactor). > > > The other is due to type differences between head & tail, which > > > causes > > > it to sometimes reuse an old buffer incorrectly. > > > > > > Patch 1&2 fix bugs > > > Patch 3 limits the size of the ring as it's not > > > possible to address more entries with 16 bit head/tail > > > > Reviewed-by: Hao Xu <howeyxu@tencent.com> > > > > > > > > I will send test cases for liburing shortly. > > > > > > One question might be if we should change the type of > > > ring_entries > > > to uint16_t in struct io_uring_buf_reg? > > > > Why not? 5.19 is just rc2 now. So we can assume there is no users > > using > > it right now I think? > > It's fine to change, but might be better if we want to extend it > in the future. Do other pbuf bits allow more than 2^16 buffers? > I guess with + if (reg.ring_entries >= 65536) + return -EINVAL; it doesn't matter either way. we can always use those bits later if we need?
On 6/13/22 14:16, Dylan Yudaken wrote: > On Mon, 2022-06-13 at 13:59 +0100, Pavel Begunkov wrote: >> On 6/13/22 12:08, Hao Xu wrote: >>> On 6/13/22 18:11, Dylan Yudaken wrote: >>>> This fixes two problems in the new provided buffer ring feature. >>>> One >>>> is a simple arithmetic bug (I think this came out from a >>>> refactor). >>>> The other is due to type differences between head & tail, which >>>> causes >>>> it to sometimes reuse an old buffer incorrectly. >>>> >>>> Patch 1&2 fix bugs >>>> Patch 3 limits the size of the ring as it's not >>>> possible to address more entries with 16 bit head/tail >>> >>> Reviewed-by: Hao Xu <howeyxu@tencent.com> >>> >>>> >>>> I will send test cases for liburing shortly. >>>> >>>> One question might be if we should change the type of >>>> ring_entries >>>> to uint16_t in struct io_uring_buf_reg? >>> >>> Why not? 5.19 is just rc2 now. So we can assume there is no users >>> using >>> it right now I think? >> >> It's fine to change, but might be better if we want to extend it >> in the future. Do other pbuf bits allow more than 2^16 buffers? >> might be better to leave it u32 * > I guess with > > + if (reg.ring_entries >= 65536) > + return -EINVAL; > > it doesn't matter either way. we can always use those bits later if we > need? I think so as well. I was also wondering whether pbufs can potentially allow >16 bits, but taking a quick look IORING_CQE_BUFFER_SHIFT=16, so we only have 16 bits in cqe::flags for indexes we return.
On 6/13/22 22:05, Pavel Begunkov wrote: > On 6/13/22 14:16, Dylan Yudaken wrote: >> On Mon, 2022-06-13 at 13:59 +0100, Pavel Begunkov wrote: >>> On 6/13/22 12:08, Hao Xu wrote: >>>> On 6/13/22 18:11, Dylan Yudaken wrote: >>>>> This fixes two problems in the new provided buffer ring feature. >>>>> One >>>>> is a simple arithmetic bug (I think this came out from a >>>>> refactor). >>>>> The other is due to type differences between head & tail, which >>>>> causes >>>>> it to sometimes reuse an old buffer incorrectly. >>>>> >>>>> Patch 1&2 fix bugs >>>>> Patch 3 limits the size of the ring as it's not >>>>> possible to address more entries with 16 bit head/tail >>>> >>>> Reviewed-by: Hao Xu <howeyxu@tencent.com> >>>> >>>>> >>>>> I will send test cases for liburing shortly. >>>>> >>>>> One question might be if we should change the type of >>>>> ring_entries >>>>> to uint16_t in struct io_uring_buf_reg? >>>> >>>> Why not? 5.19 is just rc2 now. So we can assume there is no users >>>> using >>>> it right now I think? >>> >>> It's fine to change, but might be better if we want to extend it >>> in the future. Do other pbuf bits allow more than 2^16 buffers? >>> > > might be better to leave it u32 * > >> I guess with >> >> + if (reg.ring_entries >= 65536) >> + return -EINVAL; >> >> it doesn't matter either way. we can always use those bits later if we >> need? > > I think so as well. > > I was also wondering whether pbufs can potentially allow >16 bits, > but taking a quick look IORING_CQE_BUFFER_SHIFT=16, so we only have > 16 bits in cqe::flags for indexes we return. > Yea, the 16 bits return index in cqe->flags is a hard limit for pbuf ring feature, but I do think it's ok since 1<<16 is already big