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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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!






[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux