> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Tuesday 4 October 2022 18:09
> To: Kearney, Tadhg <tadhg.kear...@intel.com>
> Cc: dev@dpdk.org; Hunt, David <david.h...@intel.com>; Burakov, Anatoly
> <anatoly.bura...@intel.com>; Pattan, Reshma <reshma.pat...@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
