Re: [dpdk-dev] [PATCH v7 2/9] eal: introduce RTE common initialization level

[Ori Kam <orika@mellanox.com>]

Hi David,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> 
> On Mon, Jul 20, 2020 at 6:48 PM Thomas Monjalon <thomas@monjalon.net>
> wrote:
> >
> > 20/07/2020 18:21, Ferruh Yigit:
> > > On 7/17/2020 2:49 PM, Parav Pandit wrote:
> > > > Currently mlx5_common uses CLASS priority to initialize
> > > > common code before initializing the PMD.
> > > > However mlx5_common is not really a class, it is the pre-initialization
> > > > code needed for the PMDs.
> > > >
> > > > In subsequent patch a needed initialization sequence is:
> > > > (a) Initialize bus (say pci)
> > > > (b) Initialize common code of a driver (mlx5_common)
> > > > (c) Register mlx5 class PMDs (mlx5 net, mlx5 vdpa)
> > > > Information registered by these PMDs is used by mlx5_bus_pci PMD.
> > > > This mlx5 class PMDs should not confused with rte_class.
> > > > (d) Register mlx5 PCI bus PMD
> > > >
> > > > Hence, introduce a new RTE priority level RTE_PRIO_COMMON which
> > > > can be used for common initialization and RTE_PRIO_CLASS by mlx5
> PMDs
> > > > for class driver initialization.
> > > >
> > > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > > ---
> > > > Changelog:
> > > > v2->v3:
> > > >  - new patch
> > > > ---
> > > >  lib/librte_eal/include/rte_common.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/lib/librte_eal/include/rte_common.h
> b/lib/librte_eal/include/rte_common.h
> > > > index 8f487a563..522afe58e 100644
> > > > --- a/lib/librte_eal/include/rte_common.h
> > > > +++ b/lib/librte_eal/include/rte_common.h
> > > > @@ -135,6 +135,7 @@ typedef uint16_t unaligned_uint16_t;
> > > >
> > > >  #define RTE_PRIORITY_LOG 101
> > > >  #define RTE_PRIORITY_BUS 110
> > > > +#define RTE_PRIORITY_COMMON 119
> > > >  #define RTE_PRIORITY_CLASS 120
> > > >  #define RTE_PRIORITY_LAST 65535
> > > >
> > > >
> > >
> > > I guess the name "common" selected because of the intention to use it by
> the
> > > common piece of the driver, but only from eal perspective the name
> > > "PRIORITY_COMMON" looks so vague, it doesn't describe any purpose.
> >
> > You're right.
> >
> > > Also the value doesn't leave any gap between the class priority, what else
> can
> > > be needed in the future in between, right?
> >
> > And we can imagine a bus requiring a common lib
> > to be initialized before.
> >
> > > @Thomas, @David, I am reluctant to get this eal change through the next-
> net, can
> > > you please review/ack it first?
> >
> > What about skipping this patch and using "RTE_PRIORITY_CLASS - 1"
> > in the code?
> 
> net and vdpa code expect the common code being initialised.
> It is a dependency internal to mlx5 drivers, I see nothing generic.
> 
First the idea was to declare a new bus not a PMD.
The issue is not from common code but from loading more than one
device on the same PCI.
So the logic is Mellanox PMD are registering to the new bus, the new bus
register to the PCI one.
So the new bus must have middle priority between the PMDs (class)  and the PCI (bus) bus.

May be a better name should have been:
RTE_PRIORITY_ MANUFACTORE _BUS  

> The common driver can provide a init function called by net and vdpa
> drivers in constructor context (not that I like calling complex init,
> but it should be equivalent to current state).
> It expresses a clear dependency and there is no constructor semantic
> to invent, nor runtime breakage possible because someone tweaks the
> priority order in the future.
> 
> There is also some hack about a haswell/broadwell detection in LOG
> priority that makes no sense.
> 
> I compile-tested following:
> 
> $ git diff next-net-mlx/master -- lib/librte_eal/ drivers/*/mlx5/
> diff --git a/drivers/common/mlx5/mlx5_common.c
> b/drivers/common/mlx5/mlx5_common.c
> index 79cd5ba344..3fb9a8ab89 100644
> --- a/drivers/common/mlx5/mlx5_common.c
> +++ b/drivers/common/mlx5/mlx5_common.c
> @@ -49,14 +49,6 @@ RTE_INIT_PRIO(mlx5_log_init, LOG)
>                 rte_log_set_level(mlx5_common_logtype, RTE_LOG_NOTICE);
>  }
> 
> -/**
> - * Initialization routine for run-time dependency on glue library.
> - */
> -RTE_INIT_PRIO(mlx5_glue_init, COMMON)
> -{
> -       mlx5_glue_constructor();
> -}
> -
>  /**
>   * This function is responsible of initializing the variable
>   *  haswell_broadwell_cpu by checking if the cpu is intel
> @@ -67,7 +59,7 @@ RTE_INIT_PRIO(mlx5_glue_init, COMMON)
>   *  if the cpu is haswell or broadwell the variable will be set to 1
>   *  otherwise it will be 0.
>   */
> -RTE_INIT_PRIO(mlx5_is_haswell_broadwell_cpu, LOG)
> +static void mlx5_is_haswell_broadwell_cpu(void)
>  {
>  #ifdef RTE_ARCH_X86_64
>         unsigned int broadwell_models[4] = {0x3d, 0x47, 0x4F, 0x56};
> @@ -114,6 +106,21 @@ RTE_INIT_PRIO(mlx5_is_haswell_broadwell_cpu,
> LOG)
>         haswell_broadwell_cpu = 0;
>  }
> 
> +/**
> + * Initialization routine for run-time dependency on glue library.
> + */
> +void mlx5_common_init(void)
> +{
> +       static bool init_once = false;
> +
> +       if (init_once)
> +               return;
> +
> +       mlx5_glue_constructor();
> +       mlx5_is_haswell_broadwell_cpu();
> +       init_once = true;
> +}
> +
>  /**
>   * Allocate page of door-bells and register it using DevX API.
>   *
> diff --git a/drivers/common/mlx5/mlx5_common.h
> b/drivers/common/mlx5/mlx5_common.h
> index 5b9b7bd5a9..30961bc8cc 100644
> --- a/drivers/common/mlx5/mlx5_common.h
> +++ b/drivers/common/mlx5/mlx5_common.h
> @@ -254,6 +254,10 @@ int64_t mlx5_get_dbr(void *ctx,  struct
> mlx5_dbr_page_list *head,
>  __rte_internal
>  int32_t mlx5_release_dbr(struct mlx5_dbr_page_list *head, uint32_t
> umem_id,
>                          uint64_t offset);
> +
> +__rte_internal
> +void mlx5_common_init(void);
> +
>  extern uint8_t haswell_broadwell_cpu;
> 
>  #endif /* RTE_PMD_MLX5_COMMON_H_ */
> diff --git a/drivers/common/mlx5/rte_common_mlx5_version.map
> b/drivers/common/mlx5/rte_common_mlx5_version.map
> index fe62fa2b2f..039d221333 100644
> --- a/drivers/common/mlx5/rte_common_mlx5_version.map
> +++ b/drivers/common/mlx5/rte_common_mlx5_version.map
> @@ -1,6 +1,7 @@
>  INTERNAL {
>         global:
> 
> +       mlx5_common_init;
>         mlx5_common_verbs_reg_mr;
>         mlx5_common_verbs_dereg_mr;
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 846398dd3d..d0e9345a55 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1989,6 +1989,8 @@ RTE_LOG_REGISTER(mlx5_logtype, pmd.net.mlx5,
> NOTICE)
>   */
>  RTE_INIT_PRIO(rte_mlx5_pmd_init, CLASS)
>  {
> +       mlx5_common_init();
> +
>         /* Build the static tables for Verbs conversion. */
>         mlx5_set_ptype_table();
>         mlx5_set_cksum_table();
> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
> index 70692ea1d2..9dc3e8fa56 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
> @@ -844,6 +844,8 @@ RTE_LOG_REGISTER(mlx5_vdpa_logtype,
> pmd.vdpa.mlx5, NOTICE)
>   */
>  RTE_INIT_PRIO(rte_mlx5_vdpa_init, CLASS)
>  {
> +       mlx5_common_init();
> +
>         if (mlx5_glue)
>                 rte_mlx5_pci_driver_register(&mlx5_vdpa_driver);
>  }
> diff --git a/lib/librte_eal/include/rte_common.h
> b/lib/librte_eal/include/rte_common.h
> index 522afe58ed..8f487a563d 100644
> --- a/lib/librte_eal/include/rte_common.h
> +++ b/lib/librte_eal/include/rte_common.h
> @@ -135,7 +135,6 @@ typedef uint16_t unaligned_uint16_t;
> 
>  #define RTE_PRIORITY_LOG 101
>  #define RTE_PRIORITY_BUS 110
> -#define RTE_PRIORITY_COMMON 119
>  #define RTE_PRIORITY_CLASS 120
>  #define RTE_PRIORITY_LAST 65535
> 
> 
> --
> David Marchand