From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67])
 by dpdk.org (Postfix) with ESMTP id 85D9B1B026
 for <dev@dpdk.org>; Mon, 19 Mar 2018 18:03:58 +0100 (CET)
Received: from lfbn-lil-1-702-109.w81-254.abo.wanadoo.fr ([81.254.39.109]
 helo=droids-corp.org)
 by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256)
 (Exim 4.89) (envelope-from <olivier.matz@6wind.com>)
 id 1exyCv-00073g-2s; Mon, 19 Mar 2018 18:04:26 +0100
Received: by droids-corp.org (sSMTP sendmail emulation);
 Mon, 19 Mar 2018 18:03:52 +0100
Date: Mon, 19 Mar 2018 18:03:52 +0100
From: Olivier Matz <olivier.matz@6wind.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: dev@dpdk.org, Bruce Richardson <bruce.richardson@intel.com>
Message-ID: <20180319170352.ddwhimxr6dzkpqea@platinum>
References: <1516713372-10572-1-git-send-email-arybchenko@solarflare.com>
 <1520696382-16400-1-git-send-email-arybchenko@solarflare.com>
 <1520696382-16400-2-git-send-email-arybchenko@solarflare.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1520696382-16400-2-git-send-email-arybchenko@solarflare.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Subject: Re: [dpdk-dev] [PATCH v1 1/9] mempool: add op to calculate memory
 size to be allocated
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://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: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 19 Mar 2018 17:03:58 -0000

On Sat, Mar 10, 2018 at 03:39:34PM +0000, Andrew Rybchenko wrote:
> Size of memory chunk required to populate mempool objects depends
> on how objects are stored in the memory. Different mempool drivers
> may have different requirements and a new operation allows to
> calculate memory size in accordance with driver requirements and
> advertise requirements on minimum memory chunk size and alignment
> in a generic way.
> 
> Bump ABI version since the patch breaks it.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

Looks good to me. Just see below for few minor comments.

> ---
> RFCv2 -> v1:
>  - move default calc_mem_size callback to rte_mempool_ops_default.c
>  - add ABI changes to release notes
>  - name default callback consistently: rte_mempool_op_<callback>_default()
>  - bump ABI version since it is the first patch which breaks ABI
>  - describe default callback behaviour in details
>  - avoid introduction of internal function to cope with depration

typo (depration)

>    (keep it to deprecation patch)
>  - move cache-line or page boundary chunk alignment to default callback
>  - highlight that min_chunk_size and align parameters are output only

[...]

> --- a/lib/librte_mempool/Makefile
> +++ b/lib/librte_mempool/Makefile
> @@ -11,11 +11,12 @@ LDLIBS += -lrte_eal -lrte_ring
>  
>  EXPORT_MAP := rte_mempool_version.map
>  
> -LIBABIVER := 3
> +LIBABIVER := 4
>  
>  # all source are stored in SRCS-y
>  SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool.c
>  SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_ops.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_ops_default.c
>  # install includes
>  SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
>  
> diff --git a/lib/librte_mempool/meson.build b/lib/librte_mempool/meson.build
> index 7a4f3da..9e3b527 100644
> --- a/lib/librte_mempool/meson.build
> +++ b/lib/librte_mempool/meson.build
> @@ -1,7 +1,8 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2017 Intel Corporation
>  
> -version = 2
> -sources = files('rte_mempool.c', 'rte_mempool_ops.c')
> +version = 4
> +sources = files('rte_mempool.c', 'rte_mempool_ops.c',
> +		'rte_mempool_ops_default.c')
>  headers = files('rte_mempool.h')
>  deps += ['ring']

It's strange to see that meson does not have the same
.so version than the legacy build system.

+CC Bruce in case he wants to fix this issue separately.

[...]

> --- a/lib/librte_mempool/rte_mempool_version.map
> +++ b/lib/librte_mempool/rte_mempool_version.map
> @@ -51,3 +51,11 @@ DPDK_17.11 {
>  	rte_mempool_populate_iova_tab;
>  
>  } DPDK_16.07;
> +
> +DPDK_18.05 {
> +	global:
> +
> +	rte_mempool_op_calc_mem_size_default;
> +
> +} DPDK_17.11;
> +

Another minor comment. When applying the patch with git am:

Applying: mempool: add op to calculate memory size to be allocated
.git/rebase-apply/patch:399: new blank line at EOF.
+
warning: 1 line adds whitespace errors.