patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Cc: Olivier Matz <olivier.matz@6wind.com>,
	dev@dpdk.org, Andrew Rybchenko <arybchenko@solarflare.com>,
	stable@dpdk.org
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] mempool: fix mempool virt populate with small chunks
Date: Thu, 9 Jan 2020 14:58:17 +0000
Message-ID: <20200109145817.GB255@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <6e43ec20-4fa5-55fa-6ca9-2ffca9bcf7dc@intel.com>

On Thu, Jan 09, 2020 at 02:29:08PM +0000, Burakov, Anatoly wrote:
> On 09-Jan-20 2:23 PM, Olivier Matz wrote:
> > On Thu, Jan 09, 2020 at 01:52:41PM +0000, Burakov, Anatoly wrote:
> > > On 09-Jan-20 1:27 PM, Olivier Matz wrote:
> > > > To populate a mempool with a virtual area, the mempool code calls
> > > > rte_mempool_populate_iova() for each iova-contiguous area. It happens
> > > > (rarely) that this area is too small to store one object. In this case,
> > > > rte_mempool_populate_iova() returns an error, which is forwarded by
> > > > rte_mempool_populate_virt().
> > > > 
> > > > This case should not throw an error in
> > > > rte_mempool_populate_virt(). Instead, the area that is too small should
> > > > just be ignored.
> > > > 
> > > > To fix this issue, change the return value of
> > > > rte_mempool_populate_iova() to -ENOBUFS when no object can be populated,
> > > > so it can be ignored by the caller. As this would be an API change, add
> > > > a compat wrapper to keep the current API unchanged. The wrapper will be
> > > > removed for 20.11.
> > > > 
> > > > Fixes: 354788b60cfd ("mempool: allow populating with unaligned virtual area")
> > > > Cc: stable@dpdk.org
> > > > 
> > > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > > ---
> > > > 
> > > 
> > > The approach fixes the issue on my end, so
> > > 
> > > Tested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > 
> > > > Is there a simple way to ensure that we won't forget to remove the
> > > > wrapper for 20.11? Anatoly suggested me to use versioned symbols, but
> > > > it's not clear to me how.
> > > > 
> > > 
> > > Yes, i'd like to do better than "ah shur we won't forget pinky swear".
> > > 
> > > Can't we do this with ABI versioning? E.g.
> > > 
> > > rte_populate_iova_v20() ... returns EINVAL
> > > 
> > > rte_populate_iova_v21() ... returns ENOBUFS
> > > 
> > > I'm pretty sure, even if it doesn't break, it will still be more likely to
> > > not be forgotten because there's almost a guarantee that someone will grep
> > > for symbol versioning macros across the codebase around 20.11 timeframe.
> > 
> > Without using symbol versionning, would this be ok too?
> > 
> >    int
> >    rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
> >           rte_iova_t iova, size_t len, rte_mempool_memchunk_free_cb_t *free_cb,
> >           void *opaque)
> >    {
> >           int ret;
> > 
> >           ret = __rte_mempool_populate_iova(mp, vaddr, iova, len, free_cb, opaque);
> > 
> >    #if RTE_VERSION < RTE_VERSION_NUM(20, 11, 0, 0)
> >           if (ret == -ENOBUFS)
> >                   ret = -EINVAL;
> >    #endif
> > 
> >           return ret;
> >    }
> > 
> > 
> 
> Well it would certainly work :) it just makes it more likely that this will
> be missed.
> 
Depends on your definition of "work". The difference vs having the ABI
versioned code is that any application linked against 20.02...20.08 will
have the old behaviour and have to change to use 20.11. Using proper
versioning, any newly linked apps immediately get the old behaviour and the
compatiblity is provided only for those apps linked against 19.11.

Therefore, I think the ABI compatibility approach is still better, and
should not be that bad, given that I think all that needs to be done for
v20 is to call v21 and then convert an -ENOBUFS to -EINVAL as you do here.

__mempool_populate_iova_v20(....)
{
   ret = __mempool_populate_iova_v21(...);
   return ret == -ENOBUFS ? -EINVAL : ret;
}

Regards,
/Bruce

  reply	other threads:[~2020-01-09 14:58 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-09 13:27 [dpdk-stable] " Olivier Matz
2020-01-09 13:40 ` [dpdk-stable] [dpdk-dev] " David Marchand
2020-01-09 13:46   ` Olivier Matz
2020-01-09 13:52 ` [dpdk-stable] " Burakov, Anatoly
2020-01-09 14:23   ` Olivier Matz
2020-01-09 14:29     ` Burakov, Anatoly
2020-01-09 14:58       ` Bruce Richardson [this message]
2020-01-17 14:57 ` [dpdk-stable] [PATCH v2 0/3] " Olivier Matz
2020-01-17 14:57   ` [dpdk-stable] [PATCH v2 1/3] " Olivier Matz
2020-01-17 14:57   ` [dpdk-stable] [PATCH v2 2/3] doc: announce API change for mempool IOVA populate Olivier Matz
2020-01-17 20:32     ` David Marchand
2020-01-17 14:57   ` [dpdk-stable] [PATCH v2 3/3] [20.05] mempool: return 0 if area is too small on populate Olivier Matz
2020-01-17 20:32     ` David Marchand
2020-01-20 12:02   ` [dpdk-stable] [dpdk-dev] [PATCH v2 0/3] mempool: fix mempool virt populate with small chunks 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=20200109145817.GB255@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=anatoly.burakov@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=olivier.matz@6wind.com \
    --cc=stable@dpdk.org \
    /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

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git