Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Jul 2023 12:37:46 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Arun KS <arunks.linux@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Arun KS <getarunks@gmail.com>, pmladek@suse.com,
        tglx@linutronix.de
Subject: Re: Question on sched_clock
Message-ID: <20230720103746.GC3569127@hirez.programming.kicks-ass.net>
References: <CAKZGPAOYPp3ANWfBWxcsT3TJdPt8jH-f2ZJzpin=UZ=-b_-QFg@mail.gmail.com>
 <CAKZGPAOY9uxvSTuta+4a41=GGqTxus8YDcv54T7iAL9P-fj_+w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAKZGPAOY9uxvSTuta+4a41=GGqTxus8YDcv54T7iAL9P-fj_+w@mail.gmail.com>
Precedence: bulk

On Thu, Jul 20, 2023 at 03:54:56PM +0530, Arun KS wrote:
> CCing maintainers
> 
> On Wed, Jul 19, 2023 at 10:36 AM Arun KS <arunks.linux@gmail.com> wrote:
> >
> > Hi,
> >
> > Kernel’s printk uses local_clock() for timestamps and it is mapped to
> > sched_clock(). Two problems/requirements I see,
> >
> > One, Kernel’s printk timestamps start from 0, I want to change this to
> > match with actual time since boot.

You can fundamentally only consistently tell time since the clock gets
initialized. Starting at 0 is what you get.

> > Two, sched_clock() doesn’t account for time spend in low power
> > state(suspend to ram)

Why would we do that? The next person will complain that they don't want
this. Then another person complains they also want time spend in
suspend-to-disk, and another person wants a pony.

> >
> > Could workout patches to modify these behaviours and found working in
> > my system. But need to hear expert opinion on why this is not done in
> > the upstream.
> >
> > diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> > index 68d6c1190ac7..b63b2ded5727 100644
> > --- a/kernel/time/sched_clock.c
> > +++ b/kernel/time/sched_clock.c

This is only one of many sched_clock implementations...

> > @@ -190,7 +190,10 @@ sched_clock_register(u64 (*read)(void), int bits,
> > unsigned long rate)
> >         /* Update epoch for new counter and update 'epoch_ns' from old counter*/
> >         new_epoch = read();
> >         cyc = cd.actual_read_sched_clock();
> > -       ns = rd.epoch_ns + cyc_to_ns((cyc - rd.epoch_cyc) &
> > rd.sched_clock_mask, rd.mult, rd.shift);
> > +       if (!cyc)
> > +               ns = cyc_to_ns(new_epoch, new_mult, new_shift)
> > +       else
> > +               ns = rd.epoch_ns + cyc_to_ns((cyc - rd.epoch_cyc) &
> > rd.sched_clock_mask, rd.mult, rd.shift);
> >         cd.actual_read_sched_clock = read;
> >
> >         rd.read_sched_clock     = read;
> >
> > @@ -287,7 +290,6 @@ void sched_clock_resume(void)
> >  {
> >         struct clock_read_data *rd = &cd.read_data[0];
> >
> > -       rd->epoch_cyc = cd.actual_read_sched_clock();

And what if you've been suspended long enough to wrap the clock ?!?

> >         hrtimer_start(&sched_clock_timer, cd.wrap_kt, HRTIMER_MODE_REL_HARD);
> >         rd->read_sched_clock = cd.actual_read_sched_clock;
> >  }
> >
> > Regards,
> > Arun