Re: [PATCH v2 1/4] sched: Cleanup qos scheduler defines from rte_config

[Thomas Monjalon <thomas@monjalon.net>]

18/02/2022 10:36, Megha Ajmera:
> Cleanup of sched config options those are by-default not defined.
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> ---
>  config/rte_config.h                        | 8 ++------
>  doc/guides/sample_app_ug/qos_scheduler.rst | 3 +--
>  lib/sched/rte_sched.c                      | 4 ++++
>  3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/config/rte_config.h b/config/rte_config.h
> index 91d96eeecb..917097630e 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -88,12 +88,8 @@
>  /* rte_power defines */
>  #define RTE_MAX_LCORE_FREQS 64
>  
> -/* rte_sched defines */
> -#undef RTE_SCHED_CMAN

So what is the purpose of the code under RTE_SCHED_CMAN #ifdef?
Is it a dead code? Should it be enabled with a hidden option?

> -#undef RTE_SCHED_COLLECT_STATS

RTE_SCHED_COLLECT_STATS should be removed from config
in the same patch removing the #ifdef in the code.

> -#undef RTE_SCHED_SUBPORT_TC_OV

RTE_SCHED_SUBPORT_TC_OV should be removed from config
in the same patch removing the #ifdef in the code.

> -#define RTE_SCHED_PORT_N_GRINDERS 8
> -#undef RTE_SCHED_VECTOR

RTE_SCHED_VECTOR should be removed while removing code in patch 4.
Maybe start the series with patch 4.
A good cleanup series starts with removing useless code.
While removing, you should justify in the commit log why it is useless.

> +/* KNI defines */
> +#define RTE_KNI_PREEMPT_DEFAULT 1

The KNI addition is unrelated.
>  
>  /* rte_graph defines */
>  #define RTE_GRAPH_BURST_SIZE 256
> diff --git a/doc/guides/sample_app_ug/qos_scheduler.rst b/doc/guides/sample_app_ug/qos_scheduler.rst
> index 49c14a00da..7016ca4078 100644
> --- a/doc/guides/sample_app_ug/qos_scheduler.rst
> +++ b/doc/guides/sample_app_ug/qos_scheduler.rst
> @@ -42,8 +42,7 @@ The application is located in the ``qos_sched`` sub-directory.
>  .. note::
>  
>      To get statistics on the sample app using the command line interface as described in the next section,
> -    DPDK must be compiled defining *RTE_SCHED_COLLECT_STATS*, which can be done by changing the relevant
> -    entry in the ``config/rte_config.h`` file.
> +    DPDK must be compiled after defining *RTE_SCHED_COLLECT_STATS* in the ``config/rte_config.h`` file.

No we should not modify rte_config.h
It should be modified via CFLAGS.

> +#ifndef RTE_SCHED_PORT_N_GRINDERS
> +#define RTE_SCHED_PORT_N_GRINDERS 8
> +#endif

Do you expect users to modify it?
Given the #ifndef, it seems yes.
So you should document it with CFLAGS.