linux-kernel - Re: [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20201201103614.GA1908@arm.com>
Date:   Tue, 1 Dec 2020 10:36:14 +0000
From:   Ionela Voinescu <ionela.voinescu@....com>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, amit.kucheria@...durent.com,
        airlied@...ux.ie, daniel.lezcano@...aro.org, steven.price@....com,
        alyssa.rosenzweig@...labora.com, rui.zhang@...el.com,
        orjan.eide@....com
Subject: Re: [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status

Hi,

Sorry for the delay and for the noise on this older version. I first
want to understand the code better.

On Thursday 22 Oct 2020 at 11:55:28 (+0100), Lukasz Luba wrote:
[..]
> 
> > 
> > > +{
> > > +	/* Make some space if needed */
> > > +	if (status->busy_time > 0xffff) {
> > > +		status->busy_time >>= 10;
> > > +		status->total_time >>= 10;
> > > +	}
> > 
> > How about removing the above code and adding here:
> > 
> > status->busy_time = status->busy_time ? : 1;
> 
> It's not equivalent. The code operates on raw device values, which
> might be big (e.g. read from counters). If it's lager than the 0xffff,
> it is going to be shifted to get smaller.
> 

Yes, the big values are handled below through the division and by making
total_time = 1024. These two initial checks are only to cover the
possibility for busy_time and total_time being 0, or busy_time >
total_time.

> > 
> > > +
> > > +	if (status->busy_time > status->total_time)
> > 
> > This check would then cover the possibility that total_time is 0.
> > 
> > > +		status->busy_time = status->total_time;
> > 
> > But a reversal is needed here:
> > 		status->total_time = status->busy_time;
> 
> No, I want to clamp the busy_time, which should not be bigger that
> total time. It could happen when we deal with 'raw' values from device
> counters.
> 

Yes, I understand. But isn't making total_time = busy_time accomplishing
the same thing?

> > 
> > > +
> > > +	status->busy_time *= 100;
> > > +	status->busy_time /= status->total_time ? : 1;
> > > +
> > > +	/* Avoid division by 0 */
> > > +	status->busy_time = status->busy_time ? : 1;
> > > +	status->total_time = 100;
> > 
> > Then all of this code can be replaced by:
> > 
> > status->busy_time = (unsigned long)div64_u64((u64)status->busy_time << 10,
> > 					     status->total_time);
> > status->total_time = 1 << 10;
> 
> No, the total_time closed to 'unsigned long' would overflow.
> 

I'm not sure I understand. total_time gets a value of 1024, it's not
itself shifted by 10.

> > 
> > This way you gain some resolution to busy_time and the divisions in the
> > callers would just become shifts by 10.
> 
> 
> I don't want to gain more resolution here. I want to be prepare for raw
> (not processed yet) big values coming from driver.
>

Agreed! The higher resolution is an extra benefit. The more important
benefit is that, through my suggestion, you'd be replacing all future
divisions by shifts.

Thanks,
Ionela.

> Regards,
> Lukasz
> 
> > 
> > Hope it helps,
> > Ionela.
> >