DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: "Kearney, Tadhg" <tadhg.kearney@intel.com>
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, 05 Oct 2022 14:11:55 +0200	[thread overview]
Message-ID: <23451635.RjEADstKbi@thomas> (raw)
In-Reply-To: <SN6PR11MB3119BC66E34C09C83C8BDA65F05D9@SN6PR11MB3119.namprd11.prod.outlook.com>

05/10/2022 12:50, Kearney, Tadhg:
> > -----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.

No, these sysfs have "intel" in their names.
So it will never become something not Intel.
At the very minimum, you should name the defines with _INTEL_

> > > + * 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.

OK better, thanks.



  reply	other threads:[~2022-10-05 12:12 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
2022-10-05 12:11             ` Thomas Monjalon [this message]
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=23451635.RjEADstKbi@thomas \
    --to=thomas@monjalon.net \
    --cc=anatoly.burakov@intel.com \
    --cc=david.hunt@intel.com \
    --cc=dev@dpdk.org \
    --cc=reshma.pattan@intel.com \
    --cc=tadhg.kearney@intel.com \
    /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).