On Wed, 2018-12-05 at 06:10 -0700, Jens Axboe wrote:
> On 12/5/18 2:56 AM, Benny Halevy wrote:
> > > +static int aio_iopoll_getevents(struct kioctx *ctx,
> > > + struct io_event __user *event,
> > > + unsigned int *nr_events, long min, long max)
> > > +{
> > > + struct aio_kiocb *iocb;
> > > + int to_poll, polled, ret;
> > > +
> > > + /*
> > > + * Check if we already have done events that satisfy what we need
> > > + */
> > > + if (!list_empty(&ctx->poll_completing)) {
> > > + ret = aio_iopoll_reap(ctx, event, nr_events, max);
> > > + if (ret < 0)
> > > + return ret;
> > > + /* if min is zero, still go through a poll check */
> >
> > A function-level comment about that would be more visible.
>
> Yes, that might be a better idea.
>
> > > + if (min && *nr_events >= min)
> > >
> > > + return 0;
> >
> > if ((min && *nr_events >= min) || *nr_events >= max) ?
> >
> > Otherwise, if poll_completing or poll_submitted are not empty,
> > besides getting to the "Shouldn't happen" case in aio_iopoll_reap,
> > it looks like we're gonna waste some cycles and never call f_op->iopoll
>
> Agree, that would be a better place to catch it. I'll make those two
> changes, thanks!
>
> > > +
> > > + /*
> > > + * Find up to 'max' worth of events to poll for, including the
> > > + * events we already successfully polled
> > > + */
> > > + polled = to_poll = 0;
> > > + list_for_each_entry(iocb, &ctx->poll_completing, ki_list) {
> > > + /*
> > > + * Poll for needed events with spin == true, anything after
> > > + * that we just check if we have more, up to max.
> > > + */
> > > + bool spin = polled + *nr_events >= min;
> >
> > I'm confused. Shouldn't spin == true if polled + *nr_events < min?
>
> Heh, that does look off, and it is. I think the correct condition is:
>
> bool spin = !polled || *nr_events < min;
>
> and I'm not sure why that got broken. I just tested it, slight
> performance win as expected correcting this. It ties in with the above
> nr_events check - it's perfectly valid to pass min_nr == 0 and just do a
> poll check. Conversely, if we already spun, just do non-spin checks on
> followup poll checks.
>
Yep, that makes sense.
> > > +static int __aio_iopoll_check(struct kioctx *ctx, struct io_event __user
> > > *event,
> > > + unsigned int *nr_events, long min_nr, long max_nr)
> > > +{
> > > + int ret = 0;
> > > +
> > > + while (!*nr_events || !need_resched()) {
> > > + int tmin = 0;
> > > +
> > > + if (*nr_events < min_nr)
> > > + tmin = min_nr - *nr_events;
> >
> > Otherwise, shouldn't the function just return 0?
> >
> > Or perhaps you explicitly want to go through the tmin==0 case
> > in aio_iopoll_getevents if *nr_events == min_nr (or min_nr==0)?
>
> No, we need to go through poll, if min_nr == 0, but only if min_nr == 0
> and !nr_events. But we only get here for nr_events != 0, so should be
> fine as-is.
>
> Thanks for your review, very useful! I'll make the above changes right
> now.
>
Thanks!
