DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier Matz <olivier.matz@6wind.com>
To: santosh <santosh.shukla@caviumnetworks.com>
Cc: dev@dpdk.org, thomas@monjalon.net, hemant.agrawal@nxp.com,
	jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [PATCH 1/4] mempool: get the external mempool capability
Date: Mon, 10 Jul 2017 15:55:56 +0200	[thread overview]
Message-ID: <20170710155556.124c98ac@platinum> (raw)
In-Reply-To: <2da743bb-9581-e181-6540-3ca9956063cb@caviumnetworks.com>

On Wed, 5 Jul 2017 12:11:52 +0530, santosh <santosh.shukla@caviumnetworks.com> 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 <santosh.shukla@caviumnetworks.com> 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 <santosh.shukla@caviumnetworks.com>
> >> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>  
> > 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.

  reply	other threads:[~2017-07-10 13:56 UTC|newest]

Thread overview: 116+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-21 17:32 [dpdk-dev] [PATCH 0/4] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-06-21 17:32 ` [dpdk-dev] [PATCH 1/4] mempool: get the external mempool capability Santosh Shukla
2017-07-03 16:37   ` Olivier Matz
2017-07-05  6:41     ` santosh
2017-07-10 13:55       ` Olivier Matz [this message]
2017-07-10 16:09         ` santosh
2017-06-21 17:32 ` [dpdk-dev] [PATCH 2/4] mempool: detect physical contiguous object in pool Santosh Shukla
2017-07-03 16:37   ` Olivier Matz
2017-07-05  7:07     ` santosh
2017-06-21 17:32 ` [dpdk-dev] [PATCH 3/4] mempool: introduce block size align flag Santosh Shukla
2017-07-03 16:37   ` Olivier Matz
2017-07-05  7:35     ` santosh
2017-07-10 13:15       ` Olivier Matz
2017-07-10 16:22         ` santosh
2017-06-21 17:32 ` [dpdk-dev] [PATCH 4/4] mempool: update range info to pool Santosh Shukla
2017-07-13  9:32 ` [dpdk-dev] [PATCH v2 0/6] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 1/6] mempool: fix flags data type Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 2/6] mempool: get the mempool capability Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 3/6] mempool: detect physical contiguous object in pool Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 4/6] mempool: add mempool arg in xmem size and usage Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 5/6] mempool: introduce block size align flag Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 6/6] mempool: update range info to pool Santosh Shukla
2017-07-18  6:07   ` [dpdk-dev] [PATCH v2 0/6] Infrastructure to support octeontx HW mempool manager santosh
2017-07-20 13:47   ` [dpdk-dev] [PATCH v3 " Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 1/6] mempool: fix flags data type Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 2/6] mempool: get the mempool capability Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 3/6] mempool: detect physical contiguous object in pool Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 4/6] mempool: add mempool arg in xmem size and usage Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 5/6] mempool: introduce block size align flag Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 6/6] mempool: update range info to pool Santosh Shukla
2017-08-15  6:07     ` [dpdk-dev] [PATCH v4 0/7] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 1/7] mempool: fix flags data type Santosh Shukla
2017-09-04 14:11         ` Olivier MATZ
2017-09-04 14:18           ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 2/7] mempool: add mempool arg in xmem size and usage Santosh Shukla
2017-09-04 14:22         ` Olivier MATZ
2017-09-04 14:33           ` santosh
2017-09-04 14:46             ` Olivier MATZ
2017-09-04 14:58               ` santosh
2017-09-04 15:23                 ` Olivier MATZ
2017-09-04 15:52                   ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 3/7] doc: remove mempool api change notice Santosh Shukla
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 4/7] mempool: get the mempool capability Santosh Shukla
2017-09-04 14:32         ` Olivier MATZ
2017-09-04 14:44           ` santosh
2017-09-04 15:56             ` Olivier MATZ
2017-09-04 16:29               ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 5/7] mempool: detect physical contiguous object in pool Santosh Shukla
2017-09-04 14:43         ` Olivier MATZ
2017-09-04 14:47           ` santosh
2017-09-04 16:00             ` Olivier MATZ
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 6/7] mempool: introduce block size align flag Santosh Shukla
2017-09-04 16:20         ` Olivier MATZ
2017-09-04 17:45           ` santosh
2017-09-07  7:27             ` Olivier MATZ
2017-09-07  7:37               ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 7/7] mempool: update range info to pool Santosh Shukla
2017-09-06 11:28       ` [dpdk-dev] [PATCH v5 0/8] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 1/8] mempool: remove unused flags argument Santosh Shukla
2017-09-07  7:41           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 2/8] mempool: change flags from int to unsigned int Santosh Shukla
2017-09-07  7:43           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 3/8] mempool: add flags arg in xmem size and usage Santosh Shukla
2017-09-07  7:46           ` Olivier MATZ
2017-09-07  7:49             ` santosh
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 4/8] doc: remove mempool notice Santosh Shukla
2017-09-07  7:47           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 5/8] mempool: get the mempool capability Santosh Shukla
2017-09-07  7:59           ` Olivier MATZ
2017-09-07  8:15             ` santosh
2017-09-07  8:39               ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 6/8] mempool: detect physical contiguous object in pool Santosh Shukla
2017-09-07  8:05           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 7/8] mempool: introduce block size align flag Santosh Shukla
2017-09-07  8:13           ` Olivier MATZ
2017-09-07  8:27             ` santosh
2017-09-07  8:57               ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 8/8] mempool: update range info to pool Santosh Shukla
2017-09-07  8:30           ` Olivier MATZ
2017-09-07  8:56             ` santosh
2017-09-07  9:09               ` Olivier MATZ
2017-09-07 15:30         ` [dpdk-dev] [PATCH v6 0/8] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 1/8] mempool: remove unused flags argument Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 2/8] mempool: change flags from int to unsigned int Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 3/8] mempool: add flags arg in xmem size and usage Santosh Shukla
2017-09-25 11:24             ` Olivier MATZ
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 4/8] doc: remove mempool notice Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 5/8] mempool: get the mempool capability Santosh Shukla
2017-09-25 11:26             ` Olivier MATZ
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 6/8] mempool: detect physical contiguous object in pool Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 7/8] mempool: introduce block size align flag Santosh Shukla
2017-09-22 12:59             ` Hemant Agrawal
2017-09-25 11:32             ` Olivier MATZ
2017-09-25 22:08               ` santosh
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 8/8] mempool: notify memory area to pool Santosh Shukla
2017-09-25 11:41             ` Olivier MATZ
2017-09-25 22:18               ` santosh
2017-09-29  4:53                 ` santosh
2017-09-29  8:20                   ` Olivier MATZ
2017-09-29  8:25                     ` santosh
2017-09-13  9:58           ` [dpdk-dev] [PATCH v6 0/8] Infrastructure to support octeontx HW mempool manager santosh
2017-09-19  8:26             ` santosh
2017-10-01  9:28           ` [dpdk-dev] [PATCH v7 " Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 1/8] mempool: remove unused flags argument Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 2/8] mempool: change flags from int to unsigned int Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 3/8] mempool: add flags arg in xmem size and usage Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 4/8] doc: remove mempool notice Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 5/8] mempool: get the mempool capability Santosh Shukla
2017-10-01  9:29             ` [dpdk-dev] [PATCH v7 6/8] mempool: detect physical contiguous object in pool Santosh Shukla
2017-10-01  9:29             ` [dpdk-dev] [PATCH v7 7/8] mempool: introduce block size align flag Santosh Shukla
2017-10-02  8:35               ` santosh
2017-10-02 14:26               ` Olivier MATZ
2017-10-01  9:29             ` [dpdk-dev] [PATCH v7 8/8] mempool: notify memory area to pool Santosh Shukla
2017-10-02  8:36               ` santosh
2017-10-02 14:27               ` Olivier MATZ
2017-10-06 20:00             ` [dpdk-dev] [PATCH v7 0/8] Infrastructure to support octeontx HW mempool manager Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170710155556.124c98ac@platinum \
    --to=olivier.matz@6wind.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerin.jacob@caviumnetworks.com \
    --cc=santosh.shukla@caviumnetworks.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).