On 11/3/24 4:17 PM, Pavel Begunkov wrote: > On 11/3/24 22:51, Jens Axboe wrote: >> On 11/3/24 3:47 PM, Pavel Begunkov wrote: >>> On 11/3/24 22:40, Jens Axboe wrote: > ... >>>> Right, but: >>>> >>>> if (current->flags & (PF_EXITING | PF_KTHREAD)) >>>> ... >>>> >>>> should be fine as it'll catch both cases with the single check. >>> >>> Was thinking to mention it, it should be fine buf feels wrong. Instead >>> of directly checking what we want, i.e. whether the task we want to run >>> the request from is dead, we are now doing "let's check if the task >>> is dead. Ah yes, let's also see if it's PF_KTHREAD which indirectly >>> implies that the task is dead because of implementation details." >>> >>> Should be fine to leave that, but why not just leave the check >>> how it was? Even if it now requires an extra deref through tctx. >> >> I think it'd be better with a comment, I added one that says: >> >> /* exiting original task or fallback work, cancel */ >> >> We can retain the original check, but it's actually a data race to check >> ->flags from a different task. Yes for this case we're in fallback work >> and the value should be long since stable, but seems prudent to just >> check for the two criteria we care about. At least the comment will be >> correct now ;-) > > I don't think whack-a-mole'ing all cases is a good thing, > but at least it can get moved into a helper and be reused in > all other places. > > if (io_tw_should_terminate(req, tw)) > fail; > > should be more readable There's only 3 spots, but yeah we can add a helper for this with a bit more of a fulfilling comment. Will do that. -- Jens Axboe
