On 1/29/19 10:26 AM, Christoph Hellwig wrote:
>> -static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx)
>> +static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
>> + struct io_submit_state *state)
>> {
>> + gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
>
> No actually in this patch, but why do we do GFP_ATOMIC allocations
> here? We aren't in irq context or under a spinlock.
Just to avoid dipping into more expensive allocs. Probably not a big
deal, I'll remove the atomic.
>
>> + if (!state)
>> + req = kmem_cache_alloc(req_cachep, gfp);
>
> Add a
> if (!req)
> goto out;
>
> plus the missing braces here..
>
>> + else if (!state->free_reqs) {
>> + size_t sz;
>> + int ret;
>> +
>> + sz = min_t(size_t, state->ios_left, ARRAY_SIZE(state->reqs));
>> + ret = kmem_cache_alloc_bulk(req_cachep, gfp, sz,
>> + state->reqs);
>> + if (ret <= 0)
>> + goto out;
>> + state->free_reqs = ret - 1;
>> + state->cur_req = 1;
>> + req = state->reqs[0];
>> + } else {
>> + req = state->reqs[state->cur_req];
>> + state->free_reqs--;
>> + state->cur_req++;
>> + }
>> +
>> if (req) {
>> req->ctx = ctx;
>> req->flags = 0;
>> return req;
>> }
>
> ... and we don't need this conditional, which would otherwise also
> be in the fast path.
Good idea, done.
--
Jens Axboe
