Re: [PATCH 3/6] bio: add allocation cache abstraction

[Jens Axboe <axboe@kernel.dk>]

On 8/12/21 1:01 AM, Christoph Hellwig wrote:
> On Wed, Aug 11, 2021 at 01:35:30PM -0600, Jens Axboe wrote:
>> +	struct bio *bio;
>> +	unsigned int i;
>> +
>> +	i = 0;
> 
> Initialize at declaration time?

Sure, done.

>> +static inline bool __bio_put(struct bio *bio)
>> +{
>> +	if (!bio_flagged(bio, BIO_REFFED))
>> +		return true;
>> +
>> +	BIO_BUG_ON(!atomic_read(&bio->__bi_cnt));
>> +
>> +	/*
>> +	 * last put frees it
>> +	 */
>> +	return atomic_dec_and_test(&bio->__bi_cnt);
>> +}
> 
> Please avoid this helper, we can trivially do the check inside of bio_put:
> 
> 	if (bio_flagged(bio, BIO_REFFED)) {
> 		BIO_BUG_ON(!atomic_read(&bio->__bi_cnt));
> 		if (!atomic_dec_and_test(&bio->__bi_cnt))
> 			return;
> 	}

Done

>> -			bio_free(bio);
>> +	if (bio_flagged(bio, BIO_PERCPU_CACHE)) {
>> +		struct bio_alloc_cache *cache;
>> +
>> +		bio_uninit(bio);
>> +		cache = per_cpu_ptr(bio->bi_pool->cache, get_cpu());
>> +		bio_list_add_head(&cache->free_list, bio);
>> +		cache->nr++;
>> +		if (cache->nr > ALLOC_CACHE_MAX + ALLOC_CACHE_SLACK)
> 
> Folding the increment as a prefix here would make the increment and test
> semantics a little more obvious.

I don't really care that deeply about it, but I generally prefer keeping
them separate as it makes it easier to read (for me). But I'll change it.

>> +struct bio *bio_alloc_kiocb(struct kiocb *kiocb, gfp_t gfp,
>> +			    unsigned short nr_vecs, struct bio_set *bs)
>> +{
>> +	struct bio_alloc_cache *cache = NULL;
>> +	struct bio *bio;
>> +
>> +	if (!(kiocb->ki_flags & IOCB_ALLOC_CACHE) || nr_vecs > BIO_INLINE_VECS)
>> +		goto normal_alloc;
>> +
>> +	cache = per_cpu_ptr(bs->cache, get_cpu());
>> +	bio = bio_list_pop(&cache->free_list);
>> +	if (bio) {
>> +		cache->nr--;
>> +		put_cpu();
>> +		bio_init(bio, nr_vecs ? bio->bi_inline_vecs : NULL, nr_vecs);
>> +		bio_set_flag(bio, BIO_PERCPU_CACHE);
>> +		return bio;
>> +	}
>> +	put_cpu();
>> +normal_alloc:
>> +	bio = bio_alloc_bioset(gfp, nr_vecs, bs);
>> +	if (cache)
>> +		bio_set_flag(bio, BIO_PERCPU_CACHE);
>> +	return bio;
> 
> The goto here is pretty obsfucating and adds an extra patch to the fast
> path.

I don't agree, and it's not the fast path - the fast path is popping off
a bio off the list, not hitting the allocator.

> Also I don't think we need the gfp argument here at all - it should
> always be GFP_KERNEL.  In fact I plan to kill these arguments from much
> of the block layer as the places that need NOIO of NOFS semantics can
> and should move to set that in the per-thread context.

Yeah, I actually kept it on purpose for future users, but let's just
kill it as both potential use cases I have use GFP_KERNEL anyway.

>> -static inline struct bio *bio_list_pop(struct bio_list *bl)
>> +static inline void bio_list_del_head(struct bio_list *bl, struct bio *head)
>>  {
>> -	struct bio *bio = bl->head;
>> -
>> -	if (bio) {
>> +	if (head) {
>>  		bl->head = bl->head->bi_next;
>>  		if (!bl->head)
>>  			bl->tail = NULL;
>>  
>> -		bio->bi_next = NULL;
>> +		head->bi_next = NULL;
>>  	}
>> +}
>> +
>> +static inline struct bio *bio_list_pop(struct bio_list *bl)
>> +{
>> +	struct bio *bio = bl->head;
>>  
>> +	bio_list_del_head(bl, bio);
>>  	return bio;
>>  }
> 
> No need for this change.

Leftover from earlier series, killed it now.

>> @@ -699,6 +706,12 @@ struct bio_set {
>>  	struct kmem_cache *bio_slab;
>>  	unsigned int front_pad;
>>  
>> +	/*
>> +	 * per-cpu bio alloc cache and notifier
>> +	 */
>> +	struct bio_alloc_cache __percpu *cache;
>> +	struct hlist_node cpuhp_dead;
>> +
> 
> I'd keep the hotplug list entry at the end instead of bloating the
> cache line used in the fast path.

Good point, moved to the end.

-- 
Jens Axboe