Re: [PATCH v2 02/20] mem: allow memseg lists to be marked as external
From: Andrew Rybchenko <hidden>
Date: 2018-09-20 09:34:12
On 9/19/18 4:56 PM, Anatoly Burakov wrote:
When we allocate and use DPDK memory, we need to be able to differentiate between DPDK hugepage segments and segments that were made part of DPDK but are externally allocated. Add such a property to memseg lists. This breaks the ABI, so bump the EAL library ABI version and document the change in release notes. All current calls for memseg walk functions were adjusted to ignore external segments where it made sense. Mempools is a special case, because we may be asked to allocate a mempool on a specific socket, and we need to ignore all page sizes on other heaps or other sockets. Previously, this assumption of knowing all page sizes was not a problem, but it will be now, so we have to match socket ID with page size when calculating minimum page size for a mempool. Signed-off-by: Anatoly Burakov <redacted>
A couple of minor questions/suggestions below, but it is OK to go as is even if rejected. Acked-by: Andrew Rybchenko <redacted> <...>
quoted hunk ↗ jump to hunk
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c index 03e6b5f73..d61c77da3 100644 --- a/lib/librte_mempool/rte_mempool.c +++ b/lib/librte_mempool/rte_mempool.c@@ -99,25 +99,40 @@ static unsigned optimize_object_size(unsigned obj_size) return new_obj_size * RTE_MEMPOOL_ALIGN; } +struct pagesz_walk_arg { + int socket_id; + size_t min; +}; + static int find_min_pagesz(const struct rte_memseg_list *msl, void *arg) { - size_t *min = arg; + struct pagesz_walk_arg *wa = arg; + bool valid; - if (msl->page_sz < *min) - *min = msl->page_sz; + valid = msl->socket_id == wa->socket_id;
Is it intended that we accept externally allocated segment if it is on requested socket? If so, it would be good to add comment to explain why.
+ valid |= wa->socket_id == SOCKET_ID_ANY && msl->external == 0; + + if (!valid) + return 0; + + if (msl->page_sz < wa->min) + wa->min = msl->page_sz;
I'd suggest to keep single return (it is just a bit shorter) if (valid && msl->page_sz < wa->min) wa->min = msl->page_sz; <...>