[PATCH -next] blk-cgroup: move congestion_count to struct blkcg

[PATCH -next] blk-cgroup: move congestion_count to struct blkcg

[Xiu Jianfeng]

The congestion_count was introduced by commit d09d8df3a294 ("blkcg:
add generic throttling mechanism"), but since it is closely related
to the blkio subsys, it is not appropriate to put it in the struct
cgroup, so move it to struct blkcg.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
only compiling tested
---
 block/blk-cgroup.c          |  4 +++-
 block/blk-cgroup.h          | 10 ++++++----
 include/linux/cgroup-defs.h |  3 ---
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 37e6cc91d576..01d3408c2fc6 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -2183,11 +2183,13 @@ void blk_cgroup_bio_start(struct bio *bio)
 bool blk_cgroup_congested(void)
 {
 	struct cgroup_subsys_state *css;
+	struct blkcg *blkcg;
 	bool ret = false;
 
 	rcu_read_lock();
 	for (css = blkcg_css(); css; css = css->parent) {
-		if (atomic_read(&css->cgroup->congestion_count)) {
+		blkcg = css_to_blkcg(css);
+		if (atomic_read(&blkcg->congestion_count)) {
 			ret = true;
 			break;
 		}
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index bd472a30bc61..16a2fbd4adca 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -95,6 +95,8 @@ struct blkcg {
 	struct cgroup_subsys_state	css;
 	spinlock_t			lock;
 	refcount_t			online_pin;
+	/* If there is block congestion on this cgroup. */
+	atomic_t congestion_count;
 
 	struct radix_tree_root		blkg_tree;
 	struct blkcg_gq	__rcu		*blkg_hint;
@@ -374,7 +376,7 @@ static inline void blkcg_use_delay(struct blkcg_gq *blkg)
 	if (WARN_ON_ONCE(atomic_read(&blkg->use_delay) < 0))
 		return;
 	if (atomic_add_return(1, &blkg->use_delay) == 1)
-		atomic_inc(&blkg->blkcg->css.cgroup->congestion_count);
+		atomic_inc(&blkg->blkcg->congestion_count);
 }
 
 static inline int blkcg_unuse_delay(struct blkcg_gq *blkg)
@@ -399,7 +401,7 @@ static inline int blkcg_unuse_delay(struct blkcg_gq *blkg)
 	if (old == 0)
 		return 0;
 	if (old == 1)
-		atomic_dec(&blkg->blkcg->css.cgroup->congestion_count);
+		atomic_dec(&blkg->blkcg->congestion_count);
 	return 1;
 }
 
@@ -418,7 +420,7 @@ static inline void blkcg_set_delay(struct blkcg_gq *blkg, u64 delay)
 
 	/* We only want 1 person setting the congestion count for this blkg. */
 	if (!old && atomic_try_cmpxchg(&blkg->use_delay, &old, -1))
-		atomic_inc(&blkg->blkcg->css.cgroup->congestion_count);
+		atomic_inc(&blkg->blkcg->congestion_count);
 
 	atomic64_set(&blkg->delay_nsec, delay);
 }
@@ -435,7 +437,7 @@ static inline void blkcg_clear_delay(struct blkcg_gq *blkg)
 
 	/* We only want 1 person clearing the congestion count for this blkg. */
 	if (old && atomic_try_cmpxchg(&blkg->use_delay, &old, 0))
-		atomic_dec(&blkg->blkcg->css.cgroup->congestion_count);
+		atomic_dec(&blkg->blkcg->congestion_count);
 }
 
 /**
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 293af7f8a694..ae04035b6cbe 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -539,9 +539,6 @@ struct cgroup {
 	/* used to store eBPF programs */
 	struct cgroup_bpf bpf;
 
-	/* If there is block congestion on this cgroup. */
-	atomic_t congestion_count;
-
 	/* Used to store internal freezer state */
 	struct cgroup_freezer_state freezer;
 
-- 
2.34.1

Re: [PATCH -next] blk-cgroup: move congestion_count to struct blkcg

[Tejun Heo]

Hello,

On Fri, Jul 12, 2024 at 08:51:41AM +0000, Xiu Jianfeng wrote:
> The congestion_count was introduced by commit d09d8df3a294 ("blkcg:
> add generic throttling mechanism"), but since it is closely related
> to the blkio subsys, it is not appropriate to put it in the struct
> cgroup, so move it to struct blkcg.
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
> only compiling tested

blkcg is per cgroup and blkg is per cgroup-device pair, so the change isn't
just moving the field but updating what it means and how it works. The
change needs a lot more thinking, justification and testing.

Thanks.

-- 
tejun

Re: [PATCH -next] blk-cgroup: move congestion_count to struct blkcg

[Yu Kuai]

Hi, Tejun!

在 2024/07/13 1:19, Tejun Heo 写道:
> Hello,
> 
> On Fri, Jul 12, 2024 at 08:51:41AM +0000, Xiu Jianfeng wrote:
>> The congestion_count was introduced by commit d09d8df3a294 ("blkcg:
>> add generic throttling mechanism"), but since it is closely related
>> to the blkio subsys, it is not appropriate to put it in the struct
>> cgroup, so move it to struct blkcg.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>> only compiling tested
> 
> blkcg is per cgroup and blkg is per cgroup-device pair, so the change isn't
> just moving the field but updating what it means and how it works. The
> change needs a lot more thinking, justification and testing
I understand blkcg and blkg, however, maybe I'm being noob, I don't see
how this patch is related to blkg, the change is that 'congestion_count'
is moved from cgroup to blkcg. This look quite straightforward to me,
maybe I'm missing something, can you explain more?

Thanks,
Kuai

> 
> Thanks.
> 

Re: [PATCH -next] blk-cgroup: move congestion_count to struct blkcg

[Tejun Heo]

Hello,

On Sat, Jul 13, 2024 at 06:56:57PM +0800, Yu Kuai wrote:
> 在 2024/07/13 1:19, Tejun Heo 写道:
> > Hello,
> > 
> > On Fri, Jul 12, 2024 at 08:51:41AM +0000, Xiu Jianfeng wrote:
> > > The congestion_count was introduced by commit d09d8df3a294 ("blkcg:
> > > add generic throttling mechanism"), but since it is closely related
> > > to the blkio subsys, it is not appropriate to put it in the struct
> > > cgroup, so move it to struct blkcg.
> > > 
> > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> > > ---
> > > only compiling tested
> > 
> > blkcg is per cgroup and blkg is per cgroup-device pair, so the change isn't
> > just moving the field but updating what it means and how it works. The
> > change needs a lot more thinking, justification and testing
> I understand blkcg and blkg, however, maybe I'm being noob, I don't see
> how this patch is related to blkg, the change is that 'congestion_count'
> is moved from cgroup to blkcg. This look quite straightforward to me,
> maybe I'm missing something, can you explain more?

Oh, my apologies. That was me confidently misreading the patch. Sorry about
that. I'll re-read the patch.

Thanks.

-- 
tejun

Re: [PATCH -next] blk-cgroup: move congestion_count to struct blkcg

[Tejun Heo]

Hello,

Sorry about the previous reply. I completely misread the patch.

On Fri, Jul 12, 2024 at 08:51:41AM +0000, Xiu Jianfeng wrote:
...
> only compiling tested

It'd be better if there's a bit more verification.

> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 37e6cc91d576..01d3408c2fc6 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -2183,11 +2183,13 @@ void blk_cgroup_bio_start(struct bio *bio)
>  bool blk_cgroup_congested(void)
>  {
>  	struct cgroup_subsys_state *css;
> +	struct blkcg *blkcg;

It'd be better to define this within the loop.

>  	bool ret = false;
>  
>  	rcu_read_lock();
>  	for (css = blkcg_css(); css; css = css->parent) {

Also, if we're now dealing with blkcg's, there's no reason to go blkcg ->
css -> blkcg again. It'd be better to get the initial blkcg and then walk up
using blkcg_parent().

> @@ -95,6 +95,8 @@ struct blkcg {
>  	struct cgroup_subsys_state	css;
>  	spinlock_t			lock;
>  	refcount_t			online_pin;
> +	/* If there is block congestion on this cgroup. */
> +	atomic_t congestion_count;

Can you please match the indentation?

Thanks.

-- 
tejun

Re: [PATCH -next] blk-cgroup: move congestion_count to struct blkcg

[xiujianfeng]

On 2024/7/14 1:41, Tejun Heo wrote:
> Hello,
> 
> Sorry about the previous reply. I completely misread the patch.
> 
> On Fri, Jul 12, 2024 at 08:51:41AM +0000, Xiu Jianfeng wrote:
> ...
>> only compiling tested
> 
> It'd be better if there's a bit more verification.
> 
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 37e6cc91d576..01d3408c2fc6 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -2183,11 +2183,13 @@ void blk_cgroup_bio_start(struct bio *bio)
>>  bool blk_cgroup_congested(void)
>>  {
>>  	struct cgroup_subsys_state *css;
>> +	struct blkcg *blkcg;
> 
> It'd be better to define this within the loop.
> 
>>  	bool ret = false;
>>  
>>  	rcu_read_lock();
>>  	for (css = blkcg_css(); css; css = css->parent) {
> 
> Also, if we're now dealing with blkcg's, there's no reason to go blkcg ->
> css -> blkcg again. It'd be better to get the initial blkcg and then walk up
> using blkcg_parent().

Thanks, will do in v2.


> 
>> @@ -95,6 +95,8 @@ struct blkcg {
>>  	struct cgroup_subsys_state	css;
>>  	spinlock_t			lock;
>>  	refcount_t			online_pin;
>> +	/* If there is block congestion on this cgroup. */
>> +	atomic_t congestion_count;
> 
> Can you please match the indentation?

Sure, I copied it from the original place, will do in v2

> 
> Thanks.
>