From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f175.google.com (mail-wr0-f175.google.com [209.85.128.175]) by dpdk.org (Postfix) with ESMTP id 039A358D1 for ; Mon, 10 Jul 2017 15:56:03 +0200 (CEST) Received: by mail-wr0-f175.google.com with SMTP id c11so139984314wrc.3 for ; Mon, 10 Jul 2017 06:56:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=1tNcWfPpTxpWZ4NfZTHODOwdl0K7KrmoMWMn4EzDu2M=; b=CcR13ahSsEcmNTIIt+QKsITIcvLEMc1Z0jFn/fpGYsQAplsQc91LXYxpWe3NBhNsta hK11d416scVlljbOvorORm4uUClMV3XDvsVjUeYRQvzEDWVo0Cjza960lvuNlkKeQ25t HxwPzm6jBH9JJyvKiSP7dl+AqMQbbMgE73rjJIuvpiGfJn0kA4z9eYusBCdcfNq//HyC AD+lxGTuW6sco7KTwNAR8ae4Kzzzd8EXspsKgOpRDqbtEUX2uIbvOwlrMpF5gh/e7ojT Ly8HUqDZbxs5vGnCvtVNw26/IIzBAZjH0mZLcJXwiiU6dq/gZ9nGftgKfvSb2MxHqCSp C6Zg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=1tNcWfPpTxpWZ4NfZTHODOwdl0K7KrmoMWMn4EzDu2M=; b=RSsgdR2ikT0hw7N4oiyfsE8Ks7F8DWePV7NDJ9aSrSPwopIMETVhDLs9i0BFjWzfTT pzBGQFOz172FQ8iupQwaKFBOMHAEJmq9O85jQRSVg1eyoFDdg0njcpCZJnNtJeFRZ0p0 Z8WWbzf8aOoOxFAvoNEgzPqvLuQ3jHhjTvc2Lfwng+SCvN9Uq69UcA/YulijmSc5caQB LKd66FDFY/O9sZtYLoW6/YPD8bSPypFSnQhguHJYleFwIUUCKvXDAT7UTqV5rsScQiO0 x2daXGmmnzyUAF0GnJW/WqwpL8qKehLFvcv5lE9Z9Qgxag3rrbYpwImux6MRYFjeyuDT Hy/A== X-Gm-Message-State: AIVw111t7TjNiZ0jVIod7qGudCz0sBCMhYR/g63GPBy5/2CTJRx+jwSx /e2z5Izt/SHbowuo X-Received: by 10.28.128.67 with SMTP id b64mr7945327wmd.79.1499694962528; Mon, 10 Jul 2017 06:56:02 -0700 (PDT) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id j31sm6191346wre.67.2017.07.10.06.56.02 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 10 Jul 2017 06:56:02 -0700 (PDT) Date: Mon, 10 Jul 2017 15:55:56 +0200 From: Olivier Matz To: santosh Cc: dev@dpdk.org, thomas@monjalon.net, hemant.agrawal@nxp.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com Message-ID: <20170710155556.124c98ac@platinum> In-Reply-To: <2da743bb-9581-e181-6540-3ca9956063cb@caviumnetworks.com> References: <20170621173248.1313-1-santosh.shukla@caviumnetworks.com> <20170621173248.1313-2-santosh.shukla@caviumnetworks.com> <20170703183705.627d185a@platinum> <2da743bb-9581-e181-6540-3ca9956063cb@caviumnetworks.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/4] mempool: get the external mempool capability X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Jul 2017 13:56:04 -0000 On Wed, 5 Jul 2017 12:11:52 +0530, santosh wrote: > Hi Olivier, > > On Monday 03 July 2017 10:07 PM, Olivier Matz wrote: > > > Hi Santosh, > > > > On Wed, 21 Jun 2017 17:32:45 +0000, Santosh Shukla wrote: > >> Allow external mempool to advertise its capability. > >> A handler been introduced called rte_mempool_ops_get_hw_cap. > >> - Upon ->get_hw_cap call, mempool driver will advertise > >> capability by returning flag. > >> - Common layer updates flag value in 'mp->flags'. > >> > >> Signed-off-by: Santosh Shukla > >> Signed-off-by: Jerin Jacob > > I guess you've already seen the compilation issue when shared libs > > are enabled: > > http://dpdk.org/dev/patchwork/patch/25603 > > > Yes, Will fix in v2. > > > > >> --- > >> lib/librte_mempool/rte_mempool.c | 5 +++++ > >> lib/librte_mempool/rte_mempool.h | 20 ++++++++++++++++++++ > >> lib/librte_mempool/rte_mempool_ops.c | 14 ++++++++++++++ > >> lib/librte_mempool/rte_mempool_version.map | 7 +++++++ > >> 4 files changed, 46 insertions(+) > >> > >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > >> index f65310f60..045baef45 100644 > >> --- a/lib/librte_mempool/rte_mempool.c > >> +++ b/lib/librte_mempool/rte_mempool.c > >> @@ -527,6 +527,11 @@ rte_mempool_populate_default(struct rte_mempool *mp) > >> if (mp->nb_mem_chunks != 0) > >> return -EEXIST; > >> > >> + /* Get external mempool capability */ > >> + ret = rte_mempool_ops_get_hw_cap(mp); > > "hw" can be removed since some handlers are software (the other occurences > > of hw should be removed too) > > > > "capabilities" is clearer than "cap" > > > > So I suggest rte_mempool_ops_get_capabilities() instead > > With this name, the comment above becomes overkill... > > ok. Will take care in v2. > > >> + if (ret != -ENOENT) > > -ENOTSUP looks more appropriate (like in ethdev) > > > imo: -ENOENT tell that driver has no new entry for capability flag(mp->flag). > But no strong opinion for -ENOTSUP. > > >> + mp->flags |= ret; > > I'm wondering if these capability flags should be mixed with > > other mempool flags. > > > > We can maybe remove this code above and directly call > > rte_mempool_ops_get_capabilities() when we need to get them. > > 0) Treating this capability flag different vs existing RTE_MEMPOLL_F would > result to adding new flag entry in struct rte_mempool { .drv_flag} for example. > 1) That new flag entry will break ABI. > 2) In-fact application can benefit this capability flag by explicitly setting > in pool create api (e.g: rte_mempool_create_empty (, , , , , _F_POOL_CONGIG | F_BLK_SZ_ALIGNED)). > > Those flag use-case not limited till driver scope, application too can benefit. > > 3) Also provided that we have space in RTE_MEMPOOL_F_XX area, so adding couple of > more bit won't impact design or effect pool creation sequence. > > 4) By calling _ops_get_capability() at _populate_default() area would address issues pointed by > you at patch [3/4]. Will explain details on ' how' in respective patch [3/4]. > > 5) Above all, Intent is to make sure that common layer managing capability flag > on behalf of driver or application. I don't see any use case where an application could request a block size alignment. The problem of adding flags that are accessible to the user is the complexity it adds to the API. If every driver comes with its own flags, I'm affraid the generic code will soon become unmaintainable. Especially, the dependencies between the flags will have to be handled somewhere. But, ok, let's do it. > >> + > >> if (rte_xen_dom0_supported()) { > >> pg_sz = RTE_PGSIZE_2M; > >> pg_shift = rte_bsf32(pg_sz); > >> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h > >> index a65f1a79d..c3cdc77e4 100644 > >> --- a/lib/librte_mempool/rte_mempool.h > >> +++ b/lib/librte_mempool/rte_mempool.h > >> @@ -390,6 +390,12 @@ typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp, > >> */ > >> typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool *mp); > >> > >> +/** > >> + * Get the mempool hw capability. > >> + */ > >> +typedef int (*rte_mempool_get_hw_cap_t)(struct rte_mempool *mp); > >> + > >> + > > If possible, use "const struct rte_mempool *mp" > > > > Since flags are unsigned, I would also prefer a function returning an > > int (0 on success, negative on error) and writing to an unsigned pointer > > provided by the user. > > > confused? mp->flag is int not unsigned. and We're returning > -ENOENT/-ENOTSUP at error and positive value in-case driver supports capability. Returing an int that is either an error or a flag mask prevents from using the last flag 0x80000000 because it is also the sign bit.