Re: [dpdk-dev] [PATCH] lib: Initial implementation of PQoS lib with CAT and CDP support

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi,

2016-01-29 13:17, Wojciech Andralojc:
>  25 files changed, 2111 insertions(+)

The patch is too big to be easily reviewed.

> +PQoS API
> +M: Kantecki, Tomasz <tomasz.kantecki@intel.com>
> +K: RTE_LIBRTE_PQOS
> +F: lib/librte_pqos/
>  
>  Drivers
>  -------
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index 74bc515..553e640 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -99,6 +99,7 @@ CONFIG_RTE_NEXT_ABI=y
>  CONFIG_RTE_LIBRTE_EAL=y
>  CONFIG_RTE_MAX_LCORE=128
>  CONFIG_RTE_MAX_NUMA_NODES=8
> +CONFIG_RTE_MAX_PHY_PKGS=8

The physical packages management should be a separate patch.

>  CONFIG_RTE_MAX_MEMSEG=256
>  CONFIG_RTE_MAX_MEMZONE=2560
>  CONFIG_RTE_MAX_TAILQ=32
> @@ -514,6 +515,13 @@ CONFIG_RTE_LIBRTE_VHOST_NUMA=n
>  CONFIG_RTE_LIBRTE_VHOST_DEBUG=n
>  
>  #
> +# Compile librte_pqos
> +# IA only
> +# LIBRTE_PQOS is for librte_pqos only

I think this line is not needed.

> +CONFIG_RTE_LIBRTE_PQOS=n
> +CONFIG_RTE_LIBRTE_PQOS_DEBUG=n

> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -346,4 +346,13 @@ int rte_eal_hugepage_init(void);
>   */
>  int rte_eal_hugepage_attach(void);
>  
> +#ifdef RTE_LIBRTE_PQOS
> +/**
> + * Get cpu physical_package_id.
> + *
> + * This function is private to the EAL.
> + */
> +unsigned eal_cpu_phy_pkg_id(unsigned lcore_id);
> +#endif

No need to restrict this function to LIBRTE_PQOS.
Maybe it should be a public API.

> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -79,6 +79,9 @@ extern struct rte_logs rte_logs;
>  #define RTE_LOGTYPE_PIPELINE 0x00008000 /**< Log related to pipeline. */
>  #define RTE_LOGTYPE_MBUF    0x00010000 /**< Log related to mbuf. */
>  #define RTE_LOGTYPE_CRYPTODEV 0x00020000 /**< Log related to cryptodev. */
> +#ifdef RTE_LIBRTE_PQOS
> +#define RTE_LOGTYPE_PQOS    0x00040000 /**< Log related to pqos. */
> +#endif

The ifdef is useless and forbidden in a header.

> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -45,6 +45,9 @@ CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
>  CFLAGS += -I$(RTE_SDK)/lib/librte_ring
>  CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
>  CFLAGS += -I$(RTE_SDK)/lib/librte_ivshmem
> +ifeq ($(CONFIG_RTE_LIBRTE_PQOS),y)
> +CFLAGS += -I$(RTE_SDK)/lib/librte_pqos
> +endif

Please make EAL independent of librte_pqos.

> +++ b/lib/librte_pqos/rte_pqos_version.map
> @@ -0,0 +1,16 @@
> +DPDK_2.3 {

It will be DPDK_16.04.

I won't comment the library itself in this version.
Please rework EAL and patch splitting first.
Thanks