[dpdk-dev,v4,2/9] lib/librte_power: add extra msg type for policies
Checks
Commit Message
Signed-off-by: Nemanja Marjanovic <nemanja.marjanovic@intel.com>
Signed-off-by: Rory Sexton <rory.sexton@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
---
lib/librte_power/channel_commands.h | 52 +++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
Comments
Hi David,
On Wednesday 04 October 2017 02:45 PM, David Hunt wrote:
> Signed-off-by: Nemanja Marjanovic <nemanja.marjanovic@intel.com>
> Signed-off-by: Rory Sexton <rory.sexton@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
my 2cent:
General comment on implementation approach:
IMO, we should avoid PMD details in common lib area.
example: file channel_commons.h has ifdef clutter referencing
i40e pmds all over.
Perhaps we should introduce opaque handle example void * or introduce pmd
specific callback/handle which points to PMD specific metadata in power library.
Example:
struct channel_packet {
void *pmd_specific_metadata;
}
Or someway via callback (I'm not sure at the moment)
so that we could hide PMD details in common area.
Thanks.
> lib/librte_power/channel_commands.h | 52 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> index 484085b..1599706 100644
> --- a/lib/librte_power/channel_commands.h
> +++ b/lib/librte_power/channel_commands.h
> @@ -46,6 +46,7 @@ extern "C" {
> /* Valid Commands */
> #define CPU_POWER 1
> #define CPU_POWER_CONNECT 2
> +#define PKT_POLICY 3
>
> /* CPU Power Command Scaling */
> #define CPU_POWER_SCALE_UP 1
> @@ -54,11 +55,62 @@ extern "C" {
> #define CPU_POWER_SCALE_MIN 4
> #define CPU_POWER_ENABLE_TURBO 5
> #define CPU_POWER_DISABLE_TURBO 6
> +#define HOURS 24
> +
> +#ifdef RTE_LIBRTE_I40E_PMD
> +#define MAX_VFS 10
> +#endif
> +
> +#define MAX_VCPU_PER_VM 8
> +
> +typedef enum {false, true} bool;
> +
> +struct t_boost_status {
> + bool tbEnabled;
> +};
> +
> +struct timer_profile {
> + int busy_hours[HOURS];
> + int quiet_hours[HOURS];
> +#ifdef RTE_LIBRTE_I40E_PMD
> + int hours_to_use_traffic_profile[HOURS];
> +#endif
> +};
> +
> +enum workload {HIGH, MEDIUM, LOW};
> +enum policy_to_use {
> +#ifdef RTE_LIBRTE_I40E_PMD
> + TRAFFIC,
> +#endif
> + TIME,
> + WORKLOAD
> +};
> +
> +#ifdef RTE_LIBRTE_I40E_PMD
> +struct traffic {
> + uint32_t min_packet_thresh;
> + uint32_t avg_max_packet_thresh;
> + uint32_t max_max_packet_thresh;
> +};
> +#endif
>
> struct channel_packet {
> uint64_t resource_id; /**< core_num, device */
> uint32_t unit; /**< scale down/up/min/max */
> uint32_t command; /**< Power, IO, etc */
> + char vm_name[32];
> +
> +#ifdef RTE_LIBRTE_I40E_PMD
> + uint64_t vfid[MAX_VFS];
> + int nb_mac_to_monitor;
> + struct traffic traffic_policy;
> +#endif
> + uint8_t vcpu_to_control[MAX_VCPU_PER_VM];
> + uint8_t num_vcpu;
> + struct timer_profile timer_policy;
> + enum workload workload;
> + enum policy_to_use policy_to_use;
> + struct t_boost_status t_boost_status;
> };
>
>
Hi Santosh,
On 4/10/2017 4:36 PM, santosh wrote:
> Hi David,
>
>
> On Wednesday 04 October 2017 02:45 PM, David Hunt wrote:
>> Signed-off-by: Nemanja Marjanovic <nemanja.marjanovic@intel.com>
>> Signed-off-by: Rory Sexton <rory.sexton@intel.com>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
> my 2cent:
> General comment on implementation approach:
> IMO, we should avoid PMD details in common lib area.
> example: file channel_commons.h has ifdef clutter referencing
> i40e pmds all over.
>
> Perhaps we should introduce opaque handle example void * or introduce pmd
> specific callback/handle which points to PMD specific metadata in power library.
>
> Example:
> struct channel_packet {
> void *pmd_specific_metadata;
> }
>
> Or someway via callback (I'm not sure at the moment)
> so that we could hide PMD details in common area.
>
> Thanks.
I would agree that PMD specific details are good left to the PMDs,
however I think that the initial
example should be OK as is, and as new PMDs are added, we can find
commonality between them
which stays in the example, and any really specific stuff can be pushed
back behind an opaque.
What about the v5 I submitted (without the #ifdef's)? Are you OK with
that for this release, and we can
fine tune as other PMDS are added in future releases?
Regards,
Dave.
Hi David,
On Thursday 05 October 2017 02:08 PM, Hunt, David wrote:
>
> Hi Santosh,
>
> On 4/10/2017 4:36 PM, santosh wrote:
>> Hi David,
>>
>>
>> On Wednesday 04 October 2017 02:45 PM, David Hunt wrote:
>>> Signed-off-by: Nemanja Marjanovic <nemanja.marjanovic@intel.com>
>>> Signed-off-by: Rory Sexton <rory.sexton@intel.com>
>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>> ---
>> my 2cent:
>> General comment on implementation approach:
>> IMO, we should avoid PMD details in common lib area.
>> example: file channel_commons.h has ifdef clutter referencing
>> i40e pmds all over.
>>
>> Perhaps we should introduce opaque handle example void * or introduce pmd
>> specific callback/handle which points to PMD specific metadata in power library.
>>
>> Example:
>> struct channel_packet {
>> void *pmd_specific_metadata;
>> }
>>
>> Or someway via callback (I'm not sure at the moment)
>> so that we could hide PMD details in common area.
>>
>> Thanks.
>
> I would agree that PMD specific details are good left to the PMDs, however I think that the initial
> example should be OK as is, and as new PMDs are added, we can find commonality between them
> which stays in the example, and any really specific stuff can be pushed back behind an opaque.
>
> What about the v5 I submitted (without the #ifdef's)? Are you OK with that for this release, and we can
> fine tune as other PMDS are added in future releases?
>
Yes. But in future releases, we should do more code clean up in power lib and example area..
meaning; current example implementation uses names like _vsi.. specific to intel NICs,
we should remove such naming and their dependency code from example area.
Thanks.
> Regards,
> Dave.
>
>
On 5/10/2017 10:21 AM, santosh wrote:
> Hi David,
>
>
> On Thursday 05 October 2017 02:08 PM, Hunt, David wrote:
>> Hi Santosh,
>>
>> On 4/10/2017 4:36 PM, santosh wrote:
>>> Hi David,
>>>
>>>
>>> On Wednesday 04 October 2017 02:45 PM, David Hunt wrote:
>>>> Signed-off-by: Nemanja Marjanovic <nemanja.marjanovic@intel.com>
>>>> Signed-off-by: Rory Sexton <rory.sexton@intel.com>
>>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>>> ---
>>> my 2cent:
>>> General comment on implementation approach:
>>> IMO, we should avoid PMD details in common lib area.
>>> example: file channel_commons.h has ifdef clutter referencing
>>> i40e pmds all over.
>>>
>>> Perhaps we should introduce opaque handle example void * or introduce pmd
>>> specific callback/handle which points to PMD specific metadata in power library.
>>>
>>> Example:
>>> struct channel_packet {
>>> void *pmd_specific_metadata;
>>> }
>>>
>>> Or someway via callback (I'm not sure at the moment)
>>> so that we could hide PMD details in common area.
>>>
>>> Thanks.
>> I would agree that PMD specific details are good left to the PMDs, however I think that the initial
>> example should be OK as is, and as new PMDs are added, we can find commonality between them
>> which stays in the example, and any really specific stuff can be pushed back behind an opaque.
>>
>> What about the v5 I submitted (without the #ifdef's)? Are you OK with that for this release, and we can
>> fine tune as other PMDS are added in future releases?
>>
> Yes. But in future releases, we should do more code clean up in power lib and example area..
> meaning; current example implementation uses names like _vsi.. specific to intel NICs,
> we should remove such naming and their dependency code from example area.
>
> Thanks.
I agree. I plan to clean up the API in the next release of DPDK. For
exmaple, there are private header files that are called rte_*.h that
expose private functions to the documentation. These need to be renamed,
as well as moving some structures around. I can also look at re-naming
some of the vsi vars to something more generic.
Thanks,
Dave.
@@ -46,6 +46,7 @@ extern "C" {
/* Valid Commands */
#define CPU_POWER 1
#define CPU_POWER_CONNECT 2
+#define PKT_POLICY 3
/* CPU Power Command Scaling */
#define CPU_POWER_SCALE_UP 1
@@ -54,11 +55,62 @@ extern "C" {
#define CPU_POWER_SCALE_MIN 4
#define CPU_POWER_ENABLE_TURBO 5
#define CPU_POWER_DISABLE_TURBO 6
+#define HOURS 24
+
+#ifdef RTE_LIBRTE_I40E_PMD
+#define MAX_VFS 10
+#endif
+
+#define MAX_VCPU_PER_VM 8
+
+typedef enum {false, true} bool;
+
+struct t_boost_status {
+ bool tbEnabled;
+};
+
+struct timer_profile {
+ int busy_hours[HOURS];
+ int quiet_hours[HOURS];
+#ifdef RTE_LIBRTE_I40E_PMD
+ int hours_to_use_traffic_profile[HOURS];
+#endif
+};
+
+enum workload {HIGH, MEDIUM, LOW};
+enum policy_to_use {
+#ifdef RTE_LIBRTE_I40E_PMD
+ TRAFFIC,
+#endif
+ TIME,
+ WORKLOAD
+};
+
+#ifdef RTE_LIBRTE_I40E_PMD
+struct traffic {
+ uint32_t min_packet_thresh;
+ uint32_t avg_max_packet_thresh;
+ uint32_t max_max_packet_thresh;
+};
+#endif
struct channel_packet {
uint64_t resource_id; /**< core_num, device */
uint32_t unit; /**< scale down/up/min/max */
uint32_t command; /**< Power, IO, etc */
+ char vm_name[32];
+
+#ifdef RTE_LIBRTE_I40E_PMD
+ uint64_t vfid[MAX_VFS];
+ int nb_mac_to_monitor;
+ struct traffic traffic_policy;
+#endif
+ uint8_t vcpu_to_control[MAX_VCPU_PER_VM];
+ uint8_t num_vcpu;
+ struct timer_profile timer_policy;
+ enum workload workload;
+ enum policy_to_use policy_to_use;
+ struct t_boost_status t_boost_status;
};