From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@intel.com>
To: Thomas Gleixner <tglx@linutronix.de>,
"jstultz@google.com" <jstultz@google.com>,
"giometti@enneenne.com" <giometti@enneenne.com>,
"corbet@lwn.net" <corbet@lwn.net>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "x86@kernel.org" <x86@kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"intel-wired-lan@lists.osuosl.org"
<intel-wired-lan@lists.osuosl.org>,
"andriy.shevchenko@linux.intel.com"
<andriy.shevchenko@linux.intel.com>,
"Dong, Eddie" <eddie.dong@intel.com>,
"Hall, Christopher S" <christopher.s.hall@intel.com>,
"Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
"davem@davemloft.net" <davem@davemloft.net>,
"alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
"joabreu@synopsys.com" <joabreu@synopsys.com>,
"mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
"perex@perex.cz" <perex@perex.cz>,
"linux-sound@vger.kernel.org" <linux-sound@vger.kernel.org>,
"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
"peter.hilber@opensynergy.com" <peter.hilber@opensynergy.com>,
"N, Pandith" <pandith.n@intel.com>,
"Mohan, Subramanian" <subramanian.mohan@intel.com>,
"T R, Thejesh Reddy" <thejesh.reddy.t.r@intel.com>
Subject: RE: [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource structure
Date: Tue, 16 Apr 2024 10:11:42 +0000 [thread overview]
Message-ID: <CY8PR11MB73648707296EE27BF4CC6F8AC4082@CY8PR11MB7364.namprd11.prod.outlook.com> (raw)
In-Reply-To: <87cyqw3nup.ffs@tglx>
> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, April 11, 2024 3:03 AM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@intel.com>;
> jstultz@google.com; giometti@enneenne.com; corbet@lwn.net; linux-
> kernel@vger.kernel.org
> Cc: x86@kernel.org; netdev@vger.kernel.org; linux-doc@vger.kernel.org; intel-
> wired-lan@lists.osuosl.org; andriy.shevchenko@linux.intel.com; Dong, Eddie
> <eddie.dong@intel.com>; Hall, Christopher S <christopher.s.hall@intel.com>;
> Brandeburg, Jesse <jesse.brandeburg@intel.com>; davem@davemloft.net;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; perex@perex.cz; linux-sound@vger.kernel.org;
> Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> peter.hilber@opensynergy.com; N, Pandith <pandith.n@intel.com>; Mohan,
> Subramanian <subramanian.mohan@intel.com>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@intel.com>; D, Lakshmi Sowjanya
> <lakshmi.sowjanya.d@intel.com>
> Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource
> structure
>
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@intel.com wrote:
> > @@ -48,6 +49,7 @@ struct module;
> > * @archdata: Optional arch-specific data
> > * @max_cycles: Maximum safe cycle value which won't
> overflow on
> > * multiplication
> > + * @freq_khz: Clocksource frequency in khz.
> > * @name: Pointer to clocksource name
> > * @list: List head for registration (internal)
> > * @rating: Rating value for selection (higher is better)
> > @@ -70,6 +72,8 @@ struct module;
> > * validate the clocksource from which the snapshot was
> > * taken.
> > * @flags: Flags describing special properties
> > + * @base: Hardware abstraction for clock on which a clocksource
> > + * is based
> > * @enable: Optional function to enable the clocksource
> > * @disable: Optional function to disable the clocksource
> > * @suspend: Optional suspend function for the clocksource
> > @@ -105,12 +109,14 @@ struct clocksource {
> > struct arch_clocksource_data archdata; #endif
> > u64 max_cycles;
> > + u32 freq_khz;
>
> Q: Why is this a bad place to add this member?
>
> A: Because it creates a 4 byte hole in the data structure.
>
> > const char *name;
> > struct list_head list;
>
> While adding it here fills a 4 byte hole.
>
> Hint:
>
> pahole -c clocksource kernel/time/clocksource.o
>
> would have told you that.
>
> > int rating;
> > enum clocksource_ids id;
> > enum vdso_clock_mode vdso_clock_mode;
> > unsigned long flags;
> > + struct clocksource_base *base;
>
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index b58dffc58a8f..2542cfefbdee 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1193,6 +1193,40 @@ static bool timestamp_in_interval(u64 start, u64
> end, u64 ts)
> > return false;
> > }
> >
> > +static bool convert_clock(u64 *val, u32 numerator, u32 denominator) {
> > + u64 rem, res;
> > +
> > + if (!numerator || !denominator)
> > + return false;
> > +
> > + res = div64_u64_rem(*val, denominator, &rem) * numerator;
> > + *val = res + div_u64(rem * numerator, denominator);
> > + return true;
> > +}
> > +
> > +static bool convert_base_to_cs(struct system_counterval_t *scv) {
> > + struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > + struct clocksource_base *base = cs->base;
> > + u32 num, den;
> > +
> > + /* The timestamp was taken from the time keeper clock source */
> > + if (cs->id == scv->cs_id)
> > + return true;
> > +
> > + /* Check whether cs_id matches the base clock */
> > + if (!base || base->id != scv->cs_id)
> > + return false;
> > +
> > + num = scv->use_nsecs ? cs->freq_khz : base->numerator;
> > + den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
> > +
> > + convert_clock(&scv->cycles, num, den);
>
> Q: Why does this ignore the return value of convert_clock() ?
>
> A: Because all drivers will correctly fill in everything.
>
> Q: Then why does convert_clock() bother to check and have a return
> value?
>
> A: Because drivers will fail to correctly fill in everything
Agreed.
Will add a check for error case:
if (!convert_clock(&scv->cycles, num, den))
return false;
Thanks,
Sowjanya
>
> Thanks,
>
> tglx
next prev parent reply other threads:[~2024-04-16 10:11 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-10 11:48 [PATCH v6 00/11] Add support for Intel PPS Generator lakshmi.sowjanya.d
2024-04-10 11:48 ` [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource structure lakshmi.sowjanya.d
2024-04-10 21:20 ` Thomas Gleixner
2024-04-16 10:10 ` D, Lakshmi Sowjanya
2024-04-10 21:32 ` Thomas Gleixner
2024-04-16 10:11 ` D, Lakshmi Sowjanya [this message]
2024-04-10 11:48 ` [PATCH v6 02/11] e1000e: remove convert_art_to_tsc() lakshmi.sowjanya.d
2024-04-10 21:42 ` Thomas Gleixner
2024-04-10 11:48 ` [PATCH v6 03/11] igc: " lakshmi.sowjanya.d
2024-04-10 11:48 ` [PATCH v6 04/11] stmmac: intel: " lakshmi.sowjanya.d
2024-04-10 11:48 ` [PATCH v6 05/11] ALSA: hda: " lakshmi.sowjanya.d
2024-04-10 11:48 ` [PATCH v6 06/11] ice/ptp: " lakshmi.sowjanya.d
2024-04-10 11:48 ` [PATCH v6 07/11] x86/tsc: Remove art to tsc conversion functions which are obsolete lakshmi.sowjanya.d
2024-04-10 11:48 ` [PATCH v6 08/11] timekeeping: Add function to convert realtime to base clock lakshmi.sowjanya.d
2024-04-10 22:15 ` Thomas Gleixner
2024-04-16 10:17 ` D, Lakshmi Sowjanya
2024-04-10 11:48 ` [PATCH v6 09/11] pps: generators: Add PPS Generator TIO Driver lakshmi.sowjanya.d
2024-04-10 22:28 ` Thomas Gleixner
2024-04-16 10:19 ` D, Lakshmi Sowjanya
2024-04-10 11:48 ` [PATCH v6 10/11] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator lakshmi.sowjanya.d
2024-04-10 11:48 ` [PATCH v6 11/11] ABI: pps: Add ABI documentation for Intel TIO lakshmi.sowjanya.d
2024-04-10 13:59 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CY8PR11MB73648707296EE27BF4CC6F8AC4082@CY8PR11MB7364.namprd11.prod.outlook.com \
--to=lakshmi.sowjanya.d@intel.com \
--cc=alexandre.torgue@foss.st.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=anthony.l.nguyen@intel.com \
--cc=christopher.s.hall@intel.com \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=eddie.dong@intel.com \
--cc=giometti@enneenne.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jesse.brandeburg@intel.com \
--cc=joabreu@synopsys.com \
--cc=jstultz@google.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sound@vger.kernel.org \
--cc=mcoquelin.stm32@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=pandith.n@intel.com \
--cc=perex@perex.cz \
--cc=peter.hilber@opensynergy.com \
--cc=subramanian.mohan@intel.com \
--cc=tglx@linutronix.de \
--cc=thejesh.reddy.t.r@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).