Re: IORING_OP_POLL_ADD/IORING_OP_POLL_REMOVE questions

[Olivier Langlois <olivier@trillion01.com>]

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

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?

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.

Note that the poll remove sqe and the following poll add sqe don't have
exactly the same user_data.

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...

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...

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!

Greetings,
Olivier