DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Tadhg Kearney <tadhg.kearney@intel.com>
Cc: dev@dpdk.org, david.hunt@intel.com, anatoly.burakov@intel.com,
	reshma.pattan@intel.com, david.marchand@redhat.com
Subject: Re: [PATCH v9 1/3] power: add Intel uncore frequency control API to power library
Date: Mon, 10 Oct 2022 14:46:17 +0200	[thread overview]
Message-ID: <10475720.aFP6jjVeTY@thomas> (raw)
In-Reply-To: <20221006093803.2076768-2-tadhg.kearney@intel.com>

06/10/2022 11:38, Tadhg Kearney:
> +API Overview for Intel Uncore
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Overview of each function in the Intel Uncore API, with explanation of what
> +they do. Each function should not be called in the fast path.
> +
> +* **Uncore Power Init**
> +    Initialize uncore power, populate frequency array and record
> +    original min & max for die on pkg.
> +
> +* **Uncore Power Exit**
> +    Exit uncore power, restoring original min & max for die on pkg.
> +
> +* **Get Uncore Power Freq**
> +    Get current uncore freq index for die on pkg.
> +
> +* **Set Uncore Power Freq**
> +    Set min & max uncore freq index for die on pkg to specified index value
> +    (min and max will be the same).
> +
> +* **Uncore Power Max**
> +    Set min & max uncore freq to maximum frequency index for die on pkg
> +    (min and max will be the same).
> +
> +* **Uncore Power Min**
> +    Set min & max uncore freq to minimum frequency index for die on pkg
> +    (min and max will be the same).
> +
> +* **Get Num Freqs**
> +    Get the number of frequencies in the index array.
> +
> +* **Get Num Pkgs**
> +    Get the number of packages (CPU's) on the system.
> +
> +* **Get Num Dies**
> +    Get the number of die's on a given package.

This is the role of doxygen documentation to explain API.
I don't understand why it is there.
Anyway I've converted it into a definition list,
which the proper RST syntax for what you do.

>          'rte_power.c',
>          'rte_power_empty_poll.c',
>          'rte_power_pmd_mgmt.c',
> +        'rte_power_intel_uncore.c',

In general, we keep such list in alphabetical order.

[...]
> +struct uncore_power_info {
> +	unsigned int die;                    /** Core die id */
> +	unsigned int pkg;                    /** Package id */
> +	uint32_t freqs[MAX_UNCORE_FREQS];/** Frequency array */
> +	uint32_t nb_freqs;                   /** Number of available freqs */
> +	FILE *f_cur_min;                     /** FD of scaling_min */
> +	FILE *f_cur_max;                     /** FD of scaling_max */
> +	uint32_t curr_idx;                   /** Freq index in freqs array */
> +	uint32_t org_min_freq;               /** Original min freq of uncore */
> +	uint32_t org_max_freq;               /** Original max freq of uncore */
> +	uint32_t init_max_freq;              /** System max uncore freq */
> +	uint32_t init_min_freq;              /** System min uncore freq */
> +} __rte_cache_aligned;

No need of doxygen syntax in a .c file.
And an alignment is wrong.

[...]
> +		RTE_LOG(DEBUG, POWER, "Invalid uncore frequency index %u, which "
> +				"should be less than %u\n", idx, ui->nb_freqs);

When you have time, it would be good to switch to dynamic logging.
See RTE_LOG_REGISTER_DEFAULT


[...]
> +#ifndef _RTE_POWER_INTEL_UNCORE_H
> +#define _RTE_POWER_INTEL_UNCORE_H

underscore prefix is reserved for reserved keywords

[...]
> +/**
> + * Exit uncore frequency management on a specific die on a package. It will restore uncore min and

Screen width is limited, while scrolling bar is infinite,
so don't hesitate to break your lines shorter when possible,
like at the end of a sentence.




  parent reply	other threads:[~2022-10-10 12:46 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
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 [this message]
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=10475720.aFP6jjVeTY@thomas \
    --to=thomas@monjalon.net \
    --cc=anatoly.burakov@intel.com \
    --cc=david.hunt@intel.com \
    --cc=david.marchand@redhat.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).