Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <b1258877-4d02-67d9-a2db-d510eadf8441@gmail.com>
Date:   Mon, 9 Oct 2023 19:38:58 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.14.0
Subject: Re: [PATCH 2/3] delayacct: convert task->delays to a object
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     bsingharora@gmail.com, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, linux-kernel@vger.kernel.org
References: <cover.1696761522.git.chunguang.xu@shopee.com>
 <531ddc82793a39f4c09316d701a4b1170bcad4ab.1696761522.git.chunguang.xu@shopee.com>
 <20231008105801.GC6320@noisy.programming.kicks-ass.net>
 <5dba07eb-88f0-bf84-494e-b979f32ad44d@gmail.com>
 <20231009084341.GA14330@noisy.programming.kicks-ass.net>
From:   brookxu <brookxu.cn@gmail.com>
In-Reply-To: <20231009084341.GA14330@noisy.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2023/10/9 16:43, Peter Zijlstra 写道:
> On Sun, Oct 08, 2023 at 07:10:01PM +0800, brookxu wrote:
>
>>>> @@ -1331,7 +1332,7 @@ struct task_struct {
>>>>    	struct page_frag		task_frag;
>>>>    #ifdef CONFIG_TASK_DELAY_ACCT
>>>> -	struct task_delay_info		*delays;
>>>> +	struct task_delay_info		delays;
>>>>    #endif
>>> Yeah, no.
>> Yes, this way will increase about 80 bytes for task_struct, about 0.85% of
>> size of task_struct, I think this just like sched_statistics, so that can
>> better support dynamically enable through sysctl.
> But it's 80 bytes 'nobody' will use. And arguably we should do the same
> with schedstats, that's default disabled and again, that's per-task
> storage nobody ever uses.
>
> Per this argument we can grow task_struct indefinitely until it
> collapses in on itself by the sheer weight of it's information density.
> Every additional field will be a smaller fraction of the total.
>
> Yes, it makes it all a little more cumbersome, but we should really not
> burden everybody with the load of some.
>
> Surely there is another solution... ?

Hi, peter:

I found another question when I tried to allocate task_delay_info on 
demand, it is hard for us to

determine whether tsk->delays is NULL due to memory failure or delayacct 
disabled, if due to

memory failure I think we should not try to allocate it again, otherwise 
we may have performance

issues, such as following code. If we limit only try once, the code is 
very trick..


bool delayacct_enter(struct task_struct *tsk)

{
         if (!static_branch_unlikely(&delayacct_key))
                 return false;

         if (tsk->delays && !IS_ERR(tsk->delays))
                 return true;

         if (!cmpxchg(&tsk->delays, NULL, -1))
                 __delayacct_tsk_init(tsk);

         return !IS_ERR_OR_NULL(tsk->delays);

}

static inline void delayacct_blkio_start(void)
{
         if (delayacct_enable(current))
                 __delayacct_blkio_start();
}

static inline void delayacct_blkio_end(struct task_struct *p)
{
         if (delayacct_enable(p))
                 __delayacct_blkio_end(p);
}


do you have any suggestion?