Message-ID: <7116a13a-6566-3cff-50ef-137d8ed0c7e4@linux.vnet.ibm.com>
Date:   Tue, 31 Jan 2023 17:57:32 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [RFC PATCH] hrtimer: interleave timers for improved single thread
 performance at low utilization
Content-Language: en-US
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     peterz@infradead.org, arjan@linux.intel.com, mingo@kernel.org,
        Srikar Dronamraju <srikar@linux.vnet.ibm.com>,
        svaidy@linux.ibm.com, linux-kernel@vger.kernel.org,
        bigeasy@linutronix.de
References: <5ae3cb09-8c9a-11e8-75a7-cc774d9bc283@linux.vnet.ibm.com>
 <877cx30xnt.ffs@tglx>
From:   shrikanth hegde <sshegde@linux.vnet.ibm.com>
In-Reply-To: <877cx30xnt.ffs@tglx>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/31/23 4:38 PM, Thomas Gleixner wrote:
> On Tue, Jan 31 2023 at 11:18, shrikanth hegde wrote:
>> As per current design of hrtimer, it uses the _softexpires to trigger the
>> timer function.  _softexpires is set as multiple of the period/interval value.
> 
> Wrong. _softexpires is _hardexpires + slack. The slack allows for
> batching which:
> 
Ok. understood.
>> This will benefit the power saving by less wakeups.
> 
This is what i thought was the rationale of _softexpires
> But that has absolutely nothing to do with your problem:
> 
Yes. 
>> Due to this, different timers of the same period/interval values align
>> and the callbacks functions will be called at the same time.
> 
> The whole point of hrtimer_forward_now() is to forward the expiry time
> of a timer with the given period so that it expires after 'now'.
> 
> That's functionality which is used by a lot of callers to implement
> proper periodic timers.
> 
>> Came up with a naive patch, more of hack.
> 
> A broken hack to be precise because any existing user of
> hrtimer_forward() will be broken by this hack.
> 
Agree. Aim was to describe the problem. 
>> Other alternative is to use a slightly modified API for cgroups, so
>> that all other timers align and wakeups remain reduced.
> 
> I'm not seeing why you need a new API for that. The problem is _NOT_ in
> the hrtimer code at all.
> 
> Lets look at the math:
> 
>     expiry = $INITIAL_EXPIRYVALUE + $N * $PERIOD
> 
> If $INITIAL_EXPIRYVALUE is the same then for all instances then
> obviously the expiry values of all instances will be all aligned on
> multiples of $PERIOD, right?
> 
> So why the heck do you need a new hrtimer API? There is an obvious
> solution, right?
> 
> Thanks,
> 
>         tglx

Thanks Thomas for taking a look and correcting wrong bits here.  The problem    
here was that $INITIAL_EXPIRYVALUE was never set. Hence the _softexpires was    
always set as the Multiple of $PERIOD.                                          
                                                                                
As Ingo Suggested this, this can be done in sched code. would need to set the    
$INITIAL_EXPIRYVALUE in start_cfs_bandwidth and Timers would interleave on the  
same logic which was described in the patch.                                    
                                                                                
No change in hrtimer code  would be needed. will send out the patch after some  
more testing.