From: "Kearney, Tadhg" <tadhg.kearney@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"Hunt, David" <david.hunt@intel.com>,
"Burakov, Anatoly" <anatoly.burakov@intel.com>,
"Pattan, Reshma" <reshma.pattan@intel.com>
Subject: RE: [PATCH v7 1/3] power: add uncore frequency control API to the power library
Date: Wed, 5 Oct 2022 10:50:05 +0000 [thread overview]
Message-ID: <SN6PR11MB3119BC66E34C09C83C8BDA65F05D9@SN6PR11MB3119.namprd11.prod.outlook.com> (raw)
In-Reply-To: <4956396.e8TTKsaY2g@thomas>
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday 4 October 2022 18:09
> To: Kearney, Tadhg <tadhg.kearney@intel.com>
> Cc: dev@dpdk.org; Hunt, David <david.hunt@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Pattan, Reshma <reshma.pattan@intel.com>
> Subject: Re: [PATCH v7 1/3] power: add uncore frequency control API to the
> power library
>
> 28/09/2022 15:30, Tadhg Kearney:
> > Add API to allow uncore frequency adjustment. This is done through
> > manipulating related uncore frequency control sysfs entries to adjust
> > the minimum and maximum uncore frequency values.
> > Nine API's are being added that are all public and experimental.
>
> You cannot introduce an API without explaining what it is about.
> Maybe I'm an idiot but I don't know what is "uncore".
> I see it is explained in the documentation, but few words in the commit
> message would not be too much.
> At least you must say it for Linux on Intel, and which feature it is controlling.
>
> > +Uncore API
> > +----------
> > +
> > +Abstract
> > +~~~~~~~~
> > +
> > +Uncore is a term used by Intel to describe the functions of a
> > +microprocessor that are not in the core, but which must be closely
> > +connected to the core to achieve high performance;
> > +L3 cache, on-die memory controller, etc.
> > +Significant power savings can be achieved by reducing the uncore
> frequency to its lowest value.
>
> So this is an Intel thing.
Yes, so far the uncore kernel implementation covers Intel hardware.
>
> > +
> > +The Linux kernel provides the driver “intel-uncore-frequency" to
> > +control the uncore frequency limits for x86 platform. The driver is
> available from kernel version 5.6 and above.
> > +Also CONFIG_INTEL_UNCORE_FREQ_CONTROL will need to be enabled in
> the kernel, which was added in 5.6.
> > +This manipulates the contest of MSR 0x620, which sets min/max of the
> uncore for the SKU.
>
> It is correctly named "intel-uncore" in the Linux kernel.
> Why not having "Intel" in the DPDK feature name?
>
> > +
> > +
> > +API Overview for Uncore
> > +~~~~~~~~~~~~~~~~~~~~~~~
>
> A blank line is missing here.
>
> > +* **Uncore Power Init**: Initialise uncore power, populate frequency
> > +array and record
> > + original min & max for pkg & die.
> > +
> > +* **Uncore Power Exit**: Exit uncore power, restoring original min & max
> for pkg & die.
> > +
> > +* **Get Uncore Power Freq**: Get current uncore freq index for pkg &
> die.
> > +
> > +* **Set Uncore Power Freq**: Set min & max uncore freq index for pkg &
> die (min and max will be the same).
> > +
> > +* **Uncore Power Max**: Set max uncore freq index for pkg & die.
> > +
> > +* **Uncore Power Min**: Set min uncore freq index for pkg & die.
> > +
> > +* **Get Num Freqs**: Get the number of frequencies in the index array.
> > +
> > +* **Get Num Pkgs**: Get the number of packages (CPUs) on the system.
> > +
> > +* **Get Num Dies**: Get the number of die's on a given package.
>
> Not sure what you are listing here. Are they functions?
> If you really want to keep a list, I suggest using a definition list available in RST
> syntax.
> If you want to provide an explanation easy to read, full sentences connecting
> things together would be better.
Agreed.
>
> > +
> > References
> > ----------
> >
> > diff --git a/doc/guides/rel_notes/release_22_11.rst
> > b/doc/guides/rel_notes/release_22_11.rst
> > index cb7677fd3c..5d3f815b54 100644
> > --- a/doc/guides/rel_notes/release_22_11.rst
> > +++ b/doc/guides/rel_notes/release_22_11.rst
> > @@ -75,6 +75,11 @@ New Features
> > * Added ``rte_event_eth_tx_adapter_instance_get`` to get Tx adapter
> > instance ID for specified ethernet device ID and Tx queue index.
> >
> > +* **Added uncore frequency control API to the power library.**
> > +
> > + Add api to allow uncore frequency adjustment. This is done through
>
> s/api/API/
>
> > + manipulating related uncore frequency control sysfs entries to
> > + adjust the minimum and maximum uncore frequency values.
>
> It is Linux-only for Intel hardware only.
>
> > --- /dev/null
> > +++ b/lib/power/rte_power_uncore.c
>
> I would add "intel" in the filename.
>
> [...]
> > +#define UNCORE_FREQUENCY_DIR
> "/sys/devices/system/cpu/intel_uncore_frequency"
> > +#define POWER_GOVERNOR_PERF "performance"
> > +#define POWER_UNCORE_SYSFILE_MAX_FREQ \
> > +
> "/sys/devices/system/cpu/intel_uncore_frequency/package_%02u_
> die_%02u/max_freq_khz"
> > +#define POWER_UNCORE_SYSFILE_MIN_FREQ \
> > +
> "/sys/devices/system/cpu/intel_uncore_frequency/package_%02u_
> die_%02u/min_freq_khz"
> > +#define POWER_UNCORE_SYSFILE_BASE_MAX_FREQ \
> > +
> "/sys/devices/system/cpu/intel_uncore_frequency/package_%02u_
> die_%02u/initial_max_freq_khz"
> > +#define POWER_UNCORE_SYSFILE_BASE_MIN_FREQ \
> > +
> "/sys/devices/system/cpu/intel_uncore_frequency/package_%02u_
> die_%02u/initial_min_freq_khz"
>
> It is for Intel CPU only, right?
Currently only Intel CPUs are covered by these sysfs entries, but it is possible that other platforms will be included in the future.
>
> > + * This function should NOT be called in the fast path.
> > + *
> > + * @param pkg
> > + * Package number.
> > + * @param die
> > + * Die number.
>
> To me it is not clear what they are.
> Is it possible to better explain "pkg" and "die" somewhere?
> Is it related to NUMA nodes?
Each NUMA node is a package, which can contain 1 or more dies. These dies are connected in a package together via the UNCORE mesh.
Dies may appear as separate NUMA nodes, or a group of dies on a packages may appear as a single NUMA node, depending on the BIOS configuration.
Header descriptions will be changed to:
* Package number. Each physical CPU in a system is referred to as a package.
* Die number. Each package can have several dies connected together via the uncore mesh.
>
Regards, Tadhg
next prev parent reply other threads:[~2022-10-05 10:50 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-20 9:48 [PATCH v4 0/3] add uncore api to be called through l3fwd-power Tadhg Kearney
2022-09-20 9:48 ` [PATCH v4 1/3] power: add uncore frequency control API to the power library Tadhg Kearney
2022-09-23 13:15 ` Hunt, David
2022-09-20 9:48 ` [PATCH v4 2/3] l3fwd-power: add option to call uncore API Tadhg Kearney
2022-09-23 13:13 ` Hunt, David
2022-09-20 9:48 ` [PATCH v4 3/3] test/power: add unit tests for " Tadhg Kearney
2022-09-23 13:15 ` Hunt, David
2022-09-27 10:09 ` [PATCH v5 0/3] add uncore api to be called through l3fwd-power Tadhg Kearney
2022-09-27 10:09 ` [PATCH v5 1/3] power: add uncore frequency control API to the power library Tadhg Kearney
2022-09-27 10:09 ` [PATCH v5 2/3] l3fwd-power: add option to call uncore API Tadhg Kearney
2022-09-27 10:09 ` [PATCH v5 3/3] test/power: add unit tests for " Tadhg Kearney
2022-09-28 9:06 ` [PATCH v6 0/3] add uncore api to be called through l3fwd-power Tadhg Kearney
2022-09-28 9:06 ` [PATCH v6 1/3] power: add uncore frequency control API to the power library Tadhg Kearney
2022-09-28 9:06 ` [PATCH v6 2/3] l3fwd-power: add option to call uncore API Tadhg Kearney
2022-09-28 12:18 ` Hunt, David
2022-09-28 9:06 ` [PATCH v6 3/3] test/power: add unit tests for " Tadhg Kearney
2022-09-28 13:30 ` [PATCH v7 0/3] add uncore api to be called through l3fwd-power Tadhg Kearney
2022-09-28 13:30 ` [PATCH v7 1/3] power: add uncore frequency control API to the power library Tadhg Kearney
2022-10-04 17:09 ` Thomas Monjalon
2022-10-05 10:50 ` Kearney, Tadhg [this message]
2022-10-05 12:11 ` Thomas Monjalon
2022-09-28 13:30 ` [PATCH v7 2/3] l3fwd-power: add option to call uncore API Tadhg Kearney
2022-09-28 13:30 ` [PATCH v7 3/3] test/power: add unit tests for " Tadhg Kearney
2022-09-29 13:27 ` [PATCH v7 0/3] add uncore api to be called through l3fwd-power Hunt, David
2022-10-05 16:20 ` [PATCH v8 0/3] add Intel " Tadhg Kearney
2022-10-05 16:20 ` [PATCH v8 1/3] power: add Intel uncore frequency control API to power library Tadhg Kearney
2022-10-05 16:20 ` [PATCH v8 2/3] l3fwd-power: add option to call uncore API Tadhg Kearney
2022-10-05 16:20 ` [PATCH v8 3/3] test/power: add unit tests for " Tadhg Kearney
2022-10-06 9:38 ` [PATCH v9 0/3] add Intel uncore api to be called through l3fwd-power Tadhg Kearney
2022-10-06 9:38 ` [PATCH v9 1/3] power: add Intel uncore frequency control API to power library Tadhg Kearney
2022-10-06 17:32 ` Stephen Hemminger
2022-10-07 10:30 ` Hunt, David
2022-10-10 12:46 ` Thomas Monjalon
2022-10-06 9:38 ` [PATCH v9 2/3] l3fwd-power: add option to call uncore API Tadhg Kearney
2022-10-06 9:38 ` [PATCH v9 3/3] test/power: add unit tests for " Tadhg Kearney
2022-10-10 12:52 ` [PATCH v9 0/3] add Intel uncore api to be called through l3fwd-power Thomas Monjalon
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=SN6PR11MB3119BC66E34C09C83C8BDA65F05D9@SN6PR11MB3119.namprd11.prod.outlook.com \
--to=tadhg.kearney@intel.com \
--cc=anatoly.burakov@intel.com \
--cc=david.hunt@intel.com \
--cc=dev@dpdk.org \
--cc=reshma.pattan@intel.com \
--cc=thomas@monjalon.net \
/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).