Re: [patch, v2] aio: allocate kiocbs in batches

[Jeff Moyer]

Andrew Morton <akpm@xxxxxxxxxx> writes:

> On Wed, 21 Sep 2011 13:16:00 -0400
> Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
>
>>
>> +/*
>> + * struct kiocb's are allocated in batches to reduce the number of
>> + * times the ctx lock is acquired and released.
>> + */
>> +#define KIOCB_BATCH_SIZE	32
>> +struct kiocb_batch {
>> +	struct list_head head;
>> +	long total;	/* number of requests passed to sys_io_submit */
>> +	long allocated;	/* number of requests allocated so far */
>> +};
>
> I don't see a reason why `total' and `allocated' need to be 64-bit. 
> Making them 32-bit results in smaller code, smaller storage, smaller
> d-cache footprint, etc.
>
> Also, they should logically be unsigned types.

The number of iocbs passed into sys_io_submit is of type long, and so
the total and the number allocated need to be of the same size.  I
considered unsigned, but seeing as the value would be capped at long, I
didn't see a real compelling reason to switch to unsigned.

Now, I suppose I could do with a single variable there, and just
decrement it as kiocbs are allocated.

>>
>> +static int kiocb_batch_refill(struct kioctx *ctx, struct kiocb_batch *batch)
>> +{
>> +	int i;
>> +	int to_alloc, avail;
>> +	bool called_fput = false;
>> +	struct kiocb *req, *n;
>> +	struct aio_ring *ring;
>> +
>> +	to_alloc = min(batch->total - batch->allocated, KIOCB_BATCH_SIZE);
>
> And this generates a compile-time warning due to the long/int mismatch.
> Did your compiler not warn here?  (And why did `to_alloc' and `i' get
> to be `int'?  The type choices are chaotic in there!)

Oops, I missed the warning.  to_alloc and i won't be very big, since
they are capped at KIOCB_BATCH_SIZE.  I could use an unsigned short.

> I'd suggest going with "unsigned" for `total' and `allocated', and make
> KIOCB_BATCH_SIZE 32U.  Then have a think about the appropriate types
> for the derived locals such as `i', `to_alloc' and `avail'.

As mentioned above, I'd like to stick with signed types, and I'll use
just a single long for the number of kiocbs left to allocate.

>> +		spin_unlock_irq(&ctx->ctx_lock);
>> +		kunmap_atomic(ring);
>
> And there's a bug.  We need to maintain the thread's atomic state
> across the kunmap_atomic().  This should have caused a might_sleep()
> runtime warning from kunmap_atomic()'s smp_processor_id() (at least). 
> That's assuming you tested on a 32-bit highmem box and were able to
> exercise this codepath, neither of which seems likely ;)

I'll fix it.  You are right, I didn't test 32-bit highmem....

>> ...
>>
>
> I wouldn't want to do the long->unsigned conversion without runtime
> testing it so can you please do a v3?

No problem.

Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/