From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH] lib: Initial implementation of PQoS lib with
	CAT and CDP support
Date: Tue, 16 Feb 2016 17:19:09 +0100
Message-ID: <1723784.P3p8rzzn5a@xps13>
References: <1454073434-92991-1-git-send-email-wojciechx.andralojc@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org
To: Wojciech Andralojc <wojciechx.andralojc@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42])
 by dpdk.org (Postfix) with ESMTP id 01523C134
 for <dev@dpdk.org>; Tue, 16 Feb 2016 17:20:41 +0100 (CET)
Received: by mail-wm0-f42.google.com with SMTP id a4so107914218wme.1
 for <dev@dpdk.org>; Tue, 16 Feb 2016 08:20:41 -0800 (PST)
In-Reply-To: <1454073434-92991-1-git-send-email-wojciechx.andralojc@intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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