Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4d4f0a3f-6906-0c9b-1b56-22b9ff8795d4@bytedance.com>
Date:   Thu, 5 Jan 2023 15:28:17 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.2.2
Subject: Re: [External] Re: [PATCH v3] blk-throtl: Introduce sync and async
 queues for blk-throtl
To:     Tejun Heo <tj@kernel.org>
Cc:     josef@toxicpanda.com, axboe@kernel.dk, cgroups@vger.kernel.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        yinxin.x@bytedance.com
References: <20221226130505.7186-1-hanjinke.666@bytedance.com>
 <Y7X5rsnYCAAYRGQd@slm.duckdns.org>
From:   hanjinke <hanjinke.666@bytedance.com>
In-Reply-To: <Y7X5rsnYCAAYRGQd@slm.duckdns.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2023/1/5 上午6:11, Tejun Heo 写道:
> Hello,
> 
> On Mon, Dec 26, 2022 at 09:05:05PM +0800, Jinke Han wrote:
>>   static void throtl_pending_timer_fn(struct timer_list *t);
>> +static inline struct bio *throtl_qnode_bio_list_pop(struct throtl_qnode *qn);
> 
> Just define it before the first usage? Also, I think it'd be fine to let the
> compiler decide whether to inline.
> 
>> +#define BLK_THROTL_SYNC(bio) (bio->bi_opf & (REQ_SYNC | REQ_META | REQ_PRIO))
> 
> Nitpick but the above is used only in one place. Does it help to define it
> as a macro?
> 
>> +/**
>> + * throtl_qnode_bio_peek - peek a bio for a qn
>> + * @qn: the qnode to peek from
>> + *
>> + * For read qn, just peek bio from the SYNC queue and return.
>> + * For write qn, we first ask the next_to_disp for bio and will pop a bio
>> + * to fill it if it's NULL. The next_to_disp is used to pin the bio for
>> + * next to dispatch. It is necessary. In the dispatching  process, a peeked
>> + * bio may can't be dispatched due to lack of budget and has to wait, the
>> + * dispatching process may give up and the spin lock of the request queue
>> + * will be released. New bio may be queued in as the spin lock were released.
>> + * When it's time to dispatch the waiting bio, another bio may be selected to
>> + * check the limit and may be dispatched. If the dispatched bio is smaller
>> + * than the waiting bio, the bandwidth may be hard to satisfied as we may
>> + * trim the slice after each dispatch.
>> + * So pinning the next_to_disp to make sure that the waiting bio and the
>> + * dispatched one later always the same one in case that the spin lock of
>> + * queue was released and re-holded.
> 
> Can you please format it better and proof-read it. I can mostly understand
> what it's saying but it can be improved quite a bit. Can you elaborate the
> starvation scenario further? What about the [a]sync queue split makes this
> more likely?
> 
>> +/**
>> + * throtl_qnode_bio_pop: pop a bio from sync/async queue
>> + * @qn: the qnode to pop a bio from
>> + *
>> + * For write io qn, the target queue to pop was determined by the disp_sync_cnt.
>> + * Try to pop bio from target queue, fetch the bio and return it when it is not
>> + * empty. If the target queue empty, pop bio from another queue instead.
> 
> How about:
> 
>          For reads, always pop from the ASYNC queue. For writes, target SYNC
>          or ASYNC queue based on disp_sync_cnt. If empty, try the other
>          queue.
> 
>> +static inline struct bio *throtl_qnode_bio_list_pop(struct throtl_qnode *qn)
>> +{
>> +	struct bio *bio;
>> +	int from = SYNC;
>> +
>> +	if (qn->disp_sync_cnt == THROTL_SYNC_FACTOR)
>> +		from = ASYNC;
> 
> ?: often is less readable but I wonder whether it'd be more readable here:
> 
>          from = qn->disp_sync_cnt == THROTL_SYNC_FACTOR ? ASYNC : SYNC;
> 
>> +
>> +	bio = bio_list_pop(&qn->bios[from]);
>> +	if (!bio) {
>> +		from = 1 - from;
>> +		bio = bio_list_pop(&qn->bios[from]);
>> +	}
>> +
>> +	if ((qn->disp_sync_cnt < THROTL_SYNC_FACTOR) &&
>> +		(from == SYNC))
> 
> Why the line break? Also, this may be more personal preference but I'm not
> sure the parentheses are helping much here.
> 
>> +		qn->disp_sync_cnt++;
>> +	else
>> +		qn->disp_sync_cnt = 0;
>> +
>> +	return bio;
>> +}
> 
> Thanks.
> 

Thanks. Your suggestion is detailed and helpful. I will accept it and 
send the v4.

Thanks.