On 5/6/21 4:17 AM, Olivier Langlois wrote: > Hi Pavel, > > On Wed, 2021-05-05 at 18:56 +0100, Pavel Begunkov wrote: >> On 5/4/21 7:06 PM, Olivier Langlois wrote: >>> >>> >>> 2. I don't understand what I am looking at. Why am I receiving a >>> completion notification for a POLL request that has just been >>> cancelled? What is the logic behind silently discarding a >>> IORING_OP_POLL_ADD sqe meant to replace an existing one? >> >> I'm lost in your message, so let's start with simple reasons. All >> requests post one CQE (almost true), including poll_remove requests. >> >> io_uring_prep_poll_remove(sqe, iouring_build_user_data(IOURING_POLL, >> fd, anfds [fd].egen)); >> // io_uring_sqe_set_data(sqe, iouring_build_user_data(IOURING_POLL, fd, >> anfds [fd].egen)); >> >> If poll remove and poll requests have identical user_data, as in >> the second (commented?) line you'll get two CQEs with that user_data. >> >> Did you check return value (in CQE) of poll remove? I'd recommend >> set its user_data to something unique. Did you consider that it >> may fail? > > Your comments does help me to see clearer! > > You are correct that setting the poll remove user_data is not done. > (hence the commented out statement for that purpose but I must have > entertain the idea to set it at some point to see what good it would > do) > > The reason being that I do not care about whether or not it succeeds > because the very next thing that I do is to rearm the poll for the same > fd with a different event mask. > > Beside, the removed poll original sqe is reported back as ECANCELED (- > 125): > 85 gen 1 res -125 That's why I mentioned setting user_data, so can distinguish cqes. > This appear to be the code returned in io_poll_remove_one() > > Does the poll remove operation generates 2 cqes? > 1 for the canceled sqe and and 1 for the poll remove sqe itself? No, only one. > > I am not too sure what good setting a distinct user_data to the poll > remove sqe could do but I will definitely give it a shot to perhaps see > clearer. again to be able to distinguish cqes, at least for debugging, but I don't see how it can be not racy without it. > Note that the poll remove sqe and the following poll add sqe don't have > exactly the same user_data. Right, I noticed. Was concerned about gen1 poll and its poll remove. > I have this statement in between: > /* increment generation counter to avoid handling old events */ > ++anfds [fd].egen; > > poll remove cancel the previous poll add having gen 1 in its user data. > the next poll add has it user_data storing gen var set to 2: > > 1 3 remove 85 1 > 1 3 add 85 2 > > 85 gen 1 res -125 > 85 gen 1 res 4 > > I'll try to be more succinct this time. > > If the poll add sqe having gen 1 in its user_data is cancelled, how can > its completion can be reported in the very next cqe? > > and I never hear back about the poll add gen 2 sqe... This one sounds like that "85 gen 1 res 4" is actually gen2 but with screwed user_data. I'd rather double check that you set it right, and don't race with multiple threads. FWIW, submission queue filling is not synchronised by liburing, users should do that. > > I'll try to get more familiar with the fs/io_uring.c code but it feels > like it could be some optimization done where because the cancelled > poll result is available while inside io_uring_enter(), instead of > discarding it to immediately rearm it for the new poll add request, > io_uring_enter() instead decide to return it back to reply to the gen 2 > request but it forgets to update the user_data field before doing so... There definitely may be a bug, but it's much more likely lurking in your code. > Maybe what is confusing is that the heading "1 3" in my traces is the > EV_READ EV_WRITE bitmask which values are: > > EV_READ = 1 > EV_WRITE = 2 > > while > > POLLIN = 1 > POLLOUT = 4 > > So my poll add gen 1 request was for be notified for POLLIN. This is > what I got with the question #1 "195" result. > > Therefore the: > 85 gen 1 res 4 > > can only be for my poll add gen 2 requesting for POLLIN|POLLOUT. Yet, > it is reported with the previous request user_data... > > I feel the mystery is almost solved with your help... I'll continue my > investigation and I'll report back if I finally solve the mystery. >> >>> 3. As I am writing this email, I have just noticed that it was >>> possible >>> to update an existing POLL entry with IORING_OP_POLL_REMOVE through >>> io_uring_prep_poll_update(). Is this what I should do to eliminate my >>> problems? What are the possible race conditions scenarios that I >>> should >>> be prepared to handle by using io_uring_prep_poll_update() (ie: >>> completion of the poll entry to update while my process is inside >>> io_uring_enter() to update it...)? >> >> Update is preferable, but it's Linux kernel 5.13. >> Both remove and update may fail. e.g. with -EALREADY >> > I am just about to install 5.12 on my system and this and the new > multishot poll feature makes me already crave 5.13! -- Pavel Begunkov
