On Tue, 2018-11-27 at 08:24 -0700, Jens Axboe wrote:
> On 11/27/18 2:53 AM, Benny Halevy wrote:
> > > @@ -818,11 +853,15 @@ static int kill_ioctx(struct mm_struct *mm, struct
> > > kioctx *ctx,
> > > {
> > > struct kioctx_table *table;
> > >
> > > + mutex_lock(&ctx->getevents_lock);
> > > spin_lock(&mm->ioctx_lock);
> > > if (atomic_xchg(&ctx->dead, 1)) {
> > > spin_unlock(&mm->ioctx_lock);
> > > + mutex_unlock(&ctx->getevents_lock);
> > > return -EINVAL;
> > > }
> > > + aio_iopoll_reap_events(ctx);
> > > + mutex_unlock(&ctx->getevents_lock);
> >
> > Is it worth handling the mutex lock and calling aio_iopoll_reap_events
> > only if (ctx->flags & IOCTX_FLAG_IOPOLL)? If so, testing it can be
> > removed from aio_iopoll_reap_events() (and maybe it could even
> > be open coded
> > here since this is its only call site apparently)
>
> I don't think it really matters, this only happens when you tear down an
> io_context. FWIW, I think it's cleaner to retain the test in the
> function, not outside it.
>
> > > @@ -1072,6 +1112,15 @@ static inline void iocb_put(struct aio_kiocb *iocb)
> > > }
> > > }
> > >
> > > +static void iocb_put_many(struct kioctx *ctx, void **iocbs, int *nr)
> > > +{
> > > + if (nr) {
> >
> > How can nr by NULL?
> > And what's the point of supporting this case?
> > Did you mean: if (*nr)?
> > (In this case, if safe to call the functions below with *nr==0,
> > I'm not sure it's worth optimizing... especially since this is a static
> > function and its callers make sure to call it only when *nr > 0)
>
> Indeed, that should be if (*nr), thanks! The slub implementation of the
> bulk free complains if you pass in nr == 0. Outside of that, a single
> check should be better than checking in multiple spots.
>
Cool. The compiler might also optimize it away when inlining this function
if the caller tests *nr for being non-zero too.
> > > @@ -1261,6 +1310,166 @@ static bool aio_read_events(struct kioctx *ctx,
> > > long min_nr, long nr,
> > > return ret < 0 || *i >= min_nr;
> > > }
> > >
> > > +#define AIO_IOPOLL_BATCH 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_IOPOLL_BATCH];
> > > + struct aio_kiocb *iocb, *n;
> > > + int to_free = 0, ret = 0;
> > > +
> > > + list_for_each_entry_safe(iocb, n, &ctx->poll_completing, ki_list) {
> > > + if (*nr_events == max)
> >
> > *nr_events >= max would be safer.
>
> I don't see how we can get there with it being larger than already, that
> would be a big bug if we fill more events than userspace asked for.
>
Currently we indeed can't, but if the code changes in the future
and we do, this will reduce the damage - hence being safer (and it costs nothing
in terms of performance).
> > > @@ -1570,17 +1829,43 @@ static inline void aio_rw_done(struct kiocb *req,
> > > ssize_t ret)
> > > default:
> > > req->ki_complete(req, ret, 0);
> > > }
> > > +
> >
> > nit: this hunk is probably unintentional
>
> Looks like it, I'll kill it.
>
>
