Hello Anatoly,
On Wed, Mar 7, 2018 at 10:26 PM, Anatoly Burakov
<anatoly.bura...@intel.com> wrote:
> If a user has specified that the zone should have contiguous memory,
> use the new _contig allocation API's instead of normal ones.
> Otherwise, account for the fact that unless we're in IOVA_AS_VA
> mode, we cannot guarantee that the pages would be physically
> contiguous, so we calculate the memzone size and alignments as if
> we were getting the smallest page size available.
>
> Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
> ---
[...]
> static void
> mempool_add_elem(struct rte_mempool *mp, void *obj, rte_iova_t iova)
> {
> @@ -549,6 +570,7 @@ rte_mempool_populate_default(struct rte_mempool *mp)
> unsigned mz_id, n;
> unsigned int mp_flags;
> int ret;
> + bool force_contig, no_contig;
>
> /* mempool must not be populated */
> if (mp->nb_mem_chunks != 0)
> @@ -563,10 +585,46 @@ rte_mempool_populate_default(struct rte_mempool *mp)
> /* update mempool capabilities */
> mp->flags |= mp_flags;
>
> - if (rte_eal_has_hugepages()) {
> - pg_shift = 0; /* not needed, zone is physically contiguous */
> + no_contig = mp->flags & MEMPOOL_F_NO_PHYS_CONTIG;
> + force_contig = mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG;
> +
> + /*
> + * there are several considerations for page size and page shift here.
> + *
> + * if we don't need our mempools to have physically contiguous
> objects,
> + * then just set page shift and page size to 0, because the user has
> + * indicated that there's no need to care about anything.
I think the above case is not handled properly here.
reason below...
> + *
> + * if we do need contiguous objects, there is also an option to
> reserve
> + * the entire mempool memory as one contiguous block of memory, in
> + * which case the page shift and alignment wouldn't matter as well.
> + *
> + * if we require contiguous objects, but not necessarily the entire
> + * mempool reserved space to be contiguous, then there are two
> options.
> + *
> + * if our IO addresses are virtual, not actual physical (IOVA as VA
> + * case), then no page shift needed - our memory allocation will give
> us
> + * contiguous physical memory as far as the hardware is concerned, so
> + * act as if we're getting contiguous memory.
> + *
> + * if our IO addresses are physical, we may get memory from bigger
> + * pages, or we might get memory from smaller pages, and how much of
> it
> + * we require depends on whether we want bigger or smaller pages.
> + * However, requesting each and every memory size is too much work, so
> + * what we'll do instead is walk through the page sizes available,
> pick
> + * the smallest one and set up page shift to match that one. We will
> be
> + * wasting some space this way, but it's much nicer than looping
> around
> + * trying to reserve each and every page size.
> + */
> +
> + if (no_contig || force_contig || rte_eal_iova_mode() == RTE_IOVA_VA) {
> pg_sz = 0;
> + pg_shift = 0;
> align = RTE_CACHE_LINE_SIZE;
So, assuming dpaa2 as example, I ran testpmd. IOVA=VA is the mode.
pg_sz = 0 is set.
same as before applying the hotplug patchset except that earlier this
decision was purely based on availability of hugepages
(rte_eal_has_hugepages()).
Moving on...
> + } else if (rte_eal_has_hugepages()) {
> + pg_sz = get_min_page_size();
> + pg_shift = rte_bsf32(pg_sz);
> + align = pg_sz;
> } else {
> pg_sz = getpagesize();
> pg_shift = rte_bsf32(pg_sz);
> @@ -585,23 +643,34 @@ rte_mempool_populate_default(struct rte_mempool *mp)
> goto fail;
> }
>
> - mz = rte_memzone_reserve_aligned(mz_name, size,
> - mp->socket_id, mz_flags, align);
> - /* not enough memory, retry with the biggest zone we have */
> - if (mz == NULL)
> - mz = rte_memzone_reserve_aligned(mz_name, 0,
> + if (force_contig) {
> + /*
> + * if contiguous memory for entire mempool memory was
> + * requested, don't try reserving again if we fail.
> + */
> + mz = rte_memzone_reserve_aligned_contig(mz_name, size,
> + mp->socket_id, mz_flags, align);
> + } else {
> + mz = rte_memzone_reserve_aligned(mz_name, size,
> mp->socket_id, mz_flags, align);
> + /* not enough memory, retry with the biggest zone we
> + * have
> + */
> + if (mz == NULL)
> + mz = rte_memzone_reserve_aligned(mz_name, 0,
> + mp->socket_id, mz_flags, align);
> + }
> if (mz == NULL) {
> ret = -rte_errno;
> goto fail;
> }
>
> - if (mp->flags & MEMPOOL_F_NO_PHYS_CONTIG)
> + if (no_contig)
> iova = RTE_BAD_IOVA;
> else
> iova = mz->iova;
>
> - if (rte_eal_has_hugepages())
> + if (rte_eal_has_hugepages() && force_contig)
So, pre-hotplugging patch, call used to enter mempool_populate_iova.
But, with the 'force_contig' not set (in app/test-pmd/testpmd.c:521)
while calling rte_pktmbuf_pool_create, rte_mempool_populate_va is
called instead.
> ret = rte_mempool_populate_iova(mp, mz->addr,
> iova, mz->len,
> rte_mempool_memchunk_mz_free,
> --
> 2.7.4
This is called with pg_sz = 0:
678 else
>># 679 ret = rte_mempool_populate_virt(mp, mz->addr,
680 mz->len, pg_sz,
681 rte_mempool_memchunk_mz_free,
682 (void *)(uintptr_t)mz);
In this function,
512 /* address and len must be page-aligned */
513 if (RTE_PTR_ALIGN_CEIL(addr, pg_sz) != addr)
514 return -EINVAL;
This is where error is returned.
I don't think RTE_PTR_ALIGN_CEIL is designed to handle pg_sz = 0.
It is roughly equivalent to:
RTE_PTR_ALIGN_FLOOR(((uintptr_t)addr - 1), pg_sz) which returns NULL
(0 ~ pg_sz).
Basically, this ends up failing rte_mempool_populate_default.
I think the reason is the assumption that when
rte_mempool_populate_virt is called, it can handle 0 page sizes (there
would issues besides the above RTE_PTR_ALIGN_CEIL as well, like a
for-loop looping over off+pg_sz), is wrong. It needs a valid page-size
value to work with (!0).
So, basically, DPAA2 is stuck with this patch because of above issue,
if I am correctly comprehending it as above.
Regards,
Shreyansh
