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

  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(-)
  

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.
  

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Friday, February 18, 2022 9:37 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> thomas@monjalon.net; david.marchand@redhat.com
> Subject: [PATCH v2 1/4] sched: Cleanup qos scheduler defines from
> rte_config
> 
> 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
> -#undef RTE_SCHED_COLLECT_STATS
> -#undef RTE_SCHED_SUBPORT_TC_OV
> -#define RTE_SCHED_PORT_N_GRINDERS 8
> -#undef RTE_SCHED_VECTOR
> +/* KNI defines */
> +#define RTE_KNI_PREEMPT_DEFAULT 1
> 
>  /* 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.
> 
>  Running the Application
>  -----------------------
> diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
> index 62b3d2e315..6c3e3bb0bf 100644
> --- a/lib/sched/rte_sched.c
> +++ b/lib/sched/rte_sched.c
> @@ -35,6 +35,10 @@
> 
>  #endif
> 
> +#ifndef RTE_SCHED_PORT_N_GRINDERS
> +#define RTE_SCHED_PORT_N_GRINDERS 8
> +#endif
> +
>  #define RTE_SCHED_TB_RATE_CONFIG_ERR          (1e-7)
>  #define RTE_SCHED_WRR_SHIFT                   3
>  #define RTE_SCHED_MAX_QUEUES_PER_TC
> RTE_SCHED_BE_QUEUES_PER_PIPE
> --
> 2.25.1

I agree with Thomas's suggestion melt this patch into the other patches.

You would need to have an additional patch for RTE_SCHED_PORT_N_GRINDERS.
  

> > +/* KNI defines */
> > +#define RTE_KNI_PREEMPT_DEFAULT 1
> 
> The KNI addition is unrelated.
> >

Why these KNI lines, hopefully it is not intentional and it will get removed in the next version?
  

> > -/* 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?
> 

We want to keep congestion management enabled through CFLAGS for now, as we have a bit more homework to do on understanding the performance impact to enable the feature at build-time; we'll follow-up on this for next release. Right, Megha?