Re: [PATCH 15/26] aio: support for IO polling

[Jens Axboe <axboe@kernel.dk>]

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