On 3/6/22 23:00, Jens Axboe wrote: > On 3/6/22 6:32 AM, Almog Khaikin wrote: >> On 3/6/22 12:35, Almog Khaikin wrote: >>> Without a full memory barrier between the store to the flags and the >>> load of the SQ tail the two operations can be reordered and this can >>> lead to a situation where the SQPOLL thread goes to sleep while the >>> application writes to the SQ tail and doesn't see the wakeup flag. >>> This memory barrier pairs with a full memory barrier in the application >>> between its store to the SQ tail and its load of the flags. >> >> The IOPOLL list is internal to the kernel, userspace doesn't interact >> with it. AFAICT it can't cause any races with userspace so the check if >> the list is empty seems unnecessary. The flags and the SQ tail are the >> only things that are shared that can cause any problems when the kernel >> thread goes to sleep so I think it's safe to remove that check. >> >> The race here can result in a situation where the kernel thread goes to >> sleep while the application updates the SQ tail and doesn't see the >> NEED_WAKEUP flag. Checking the SQ tail after setting the wakeup flag >> along with the full barrier would ensure that either we see the tail >> update or the application sees the wakeup flag. The IOPOLL list doesn't >> tie into any of this. > > I think you're mixing up two different things, and even if not, the > IOPOLL change should be a separate change. > > The iopoll list check isn't about synchronizing with userspace, it's > about not going to sleep if we have entries to reap. If we're running > with IOPOLL|SQPOLL, then it's the sq poll thread that does the polling > and reaping. I understand that, but the iopoll list check is already done earlier in the function and if the list isn't empty, the timer is reset. Checking again just a little later in the function and after writing the NEED_WAKEUP flag seems unnecessary but regardless, I guess it's not really relevant to this patch as it's not a bug that the check is there. The patch in the original message along with the liburing pull request should fix the race.
