Re: [dpdk-dev] [PATCH] mempool: fix mempool virt populate with small chunks

[Bruce Richardson <bruce.richardson@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