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.
>> +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.
--
Jens Axboe
