linux-kernel - Re: [PATCH] blk-throttle: check policy bit in blk_throtl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c60c0768-2b1b-a26b-db7d-340fd29ff688@huaweicloud.com>
Date: Wed, 3 Sep 2025 14:03:37 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Han Guangjiang <gj.han@...mail.com>, hailan@...uai.org.cn
Cc: axboe@...nel.dk, fanggeng@...iang.com, hanguangjiang@...iang.com,
 liangjie@...iang.com, linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org, yangchen11@...iang.com,
 "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

Hi,

在 2025/09/03 10:55, Han Guangjiang 写道:
> Hi Kuai,
> 
>> Instead of add checking from hot path, do you consider delaying setting q->td
>> until policy is activated from the slow path? I think this is better solution.
> 
> Thank you for your review. You're absolutely right that performance
> considerations in the hot path are important.
> 
> We actually considered delaying the setting of q->td until after policy
> activation, but we found that q->td is needed by blkcg_activate_policy()
> during its execution, so it has to be set before calling
> blkcg_activate_policy().

That's not hard to bypass, q->td is used to initialze tg->td in
throtl_pd_init(), actually you can just remove it, and add a helper
tg_to_td() to replace it;

struct throtl_data *tg_to_td(struct throtl_grp *tg)
{
	return tg_to_blkg(tg)->q->td;
}

Meanwhile, please remove the comment about freeze queue, turns out it
can't protect blk_throtl_bio() becasue q_usage_coutner is not grabbed
yet while issuing bio.

Thanks,
Kuai

> 
> We explored several alternative approaches:
> 
> 1) Adding a dedicated flag like 'throttle_ready' to struct request_queue:
>     - Set this flag at the end of blk_throtl_init()
>     - Check this flag in blk_throtl_activated() to determine if policy
>       loading is complete
>     - However, this requires adding a new bool variable to the struct
> 
> 2) Reusing the q->td pointer with low-order bit flags:
>     - Use pointer low-order bits to mark initialization completion status
>     - This would avoid adding new fields but requires careful handling
>       and additional processing
> 
> Given these constraints, we chose the current approach of checking the
> policy bit in blk_throtl_activated() as it:
> - Doesn't require struct changes
> - Provides a clean, atomic check
> - Aligns with the existing policy activation mechanism
> 
> We would appreciate your suggestions on how to better handle this
> initialization race condition.
> 
> Thanks,
> Han Guangjiang
> 
> 
> 
> .
>