Re: [PATCH 8/8] aio: support for IO polling

[Jens Axboe <axboe@kernel.dk>]

On 11/21/18 4:12 AM, Benny Halevy wrote:
>> +#define AIO_POLL_STACK	8
>> +
>> +/*
>> + * Process completed iocb iopoll entries, copying the result to userspace.
>> + */
>> +static long aio_iopoll_reap(struct kioctx *ctx, struct io_event __user *evs,
>> +			    unsigned int *nr_events, long max)
>> +{
>> +	void *iocbs[AIO_POLL_STACK];
>> +	struct aio_kiocb *iocb, *n;
>> +	int to_free = 0, ret = 0;
> 
> To be on the safe side, how about checking that if (evs)
> *nr_events < max, otherwise, return -EINVAL?

Good point, I think we should re-arrange the loop a bit to move the
check up at the top to guard for entries == max at entry. I've done
that.

>> +	/*
>> +	 * Take in a new working set from the submitted list if possible.
>> +	 */
>> +	if (!list_empty_careful(&ctx->poll_submitted)) {
>> +		spin_lock(&ctx->poll_lock);
>> +		list_splice_init(&ctx->poll_submitted, &ctx->poll_completing);
>> +		spin_unlock(&ctx->poll_lock);
>> +	}
>> +
>> +	if (list_empty(&ctx->poll_completing))
>> +		return 0;
> 
> Could be somewhat optimized like this:
> 
> 	if (list_empty_careful(&ctx->poll_submitted))
> 		return 0;
> 
> 	spin_lock(&ctx->poll_lock);
> 	list_splice_init(&ctx->poll_submitted, &ctx->poll_completing);
> 	spin_unlock(&ctx->poll_lock);
> 	if (list_empty(&ctx->poll_completing))
> 		return 0;
> 
> Or, possibly...
> 	if (list_empty_careful(&ctx->poll_submitted) ||
> 	    ({
> 		spin_lock(&ctx->poll_lock);
> 		list_splice_init(&ctx->poll_submitted, &ctx->poll_completing);
> 		spin_unlock(&ctx->poll_lock);
> 		list_empty(&ctx->poll_completing);
> 	    }))
> 		return 0;

I think the readability of the existing version is better.

>> +	/*
>> +	 * Check again now that we have a new batch.
>> +	 */
>> +	ret = aio_iopoll_reap(ctx, event, nr_events, max);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (*nr_events >= min)
>> +		return 0;
>> +
>> +	/*
>> +	 * Find up to 'max_nr' worth of events to poll for, including the
> 
> What's max_nr? You mean 'max'?

It should, corrected.

>> +	 * events we already successfully polled
>> +	 */
>> +	polled = to_poll = 0;
>> +	poll_completed = atomic_read(&ctx->poll_completed);
>> +	list_for_each_entry(iocb, &ctx->poll_completing, ki_list) {
>> +		/*
>> +		 * Poll for needed events with wait == true, anything after
>> +		 * that we just check if we have more, up to max.
>> +		 */
>> +		bool wait = polled + *nr_events >= min;
>> +		struct kiocb *kiocb = &iocb->rw;
>> +
>> +		if (test_bit(IOCB_POLL_COMPLETED, &iocb->ki_flags))
>> +			break;
>> +		if (++to_poll + *nr_events >= max)
>> +			break;
>> +
>> +		polled += kiocb->ki_filp->f_op->iopoll(kiocb, wait);
> 
> Could iopoll return a negative value? (Currently not in this patchset,
> but would it be possible in the future?)

That's a good point, I've added a separate check for this. Given that
it's a regular fops handler, it should be perfectly valid to return
-ERROR.

>> +		if (polled + *nr_events >= max)
>> +			break;
>> +		if (poll_completed != atomic_read(&ctx->poll_completed))
>> +			break;
>> +	}
>> +
>> +	ret = aio_iopoll_reap(ctx, event, nr_events, max);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (*nr_events >= min)
>> +		return 0;
>> +	return to_poll;
> 
> What does the returned value mean?
> If the intention is only to return a value greater than zero,
> how about just returning to_poll > 0?

It just means that you could call us again, if > 0, and < 0 is an error
specifically.

>> +/*
>> + * We can't just wait for polled events to come to us, we have to actively
>> + * find and complete them.
>> + */
>> +static void aio_iopoll_reap_events(struct kioctx *ctx)
>> +{
>> +	if (!(ctx->flags & IOCTX_FLAG_IOPOLL))
>> +		return;
>> +
>> +	while (!list_empty_careful(&ctx->poll_submitted) ||
>> +	       !list_empty(&ctx->poll_completing)) {
>> +		unsigned int nr_events = 0;
>> +
>> +		__aio_iopoll_check(ctx, NULL, &nr_events, 1, UINT_MAX);
> 
> BUG_ON(__aoi_iopoll_check() < 0) ?

Ho hum...

>> +	}
>> +}
>> +
>> +static int aio_iopoll_check(struct kioctx *ctx, long min_nr, long nr,
>> +			    struct io_event __user *event)
>> +{
>> +	unsigned int nr_events = 0;
>> +	int ret = 0;
>> +
>> +	/* * Only allow one thread polling at a time */
> 
> nit: extra '* '

Removed.

Thanks for your review!

-- 
Jens Axboe