[dpdk-stable] [PATCH 1/2] eal/malloc: merge malloc_elems in heap if they are contiguous

[dpdk-stable] [PATCH 1/2] eal/malloc: merge malloc_elems in heap if they are contiguous

[Gowrishankar]

From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

During malloc heap init, if there are malloc_elems contiguous in
virt addresses, they could be merged so that, merged malloc_elem
would guarantee larger free memory size than its actual hugepage
size, it was created for.

Fixes: fafcc11985 ("mem: rework memzone to be allocated by malloc")
Cc: stable@dpdk.org

Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
---
 lib/librte_eal/common/malloc_heap.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
index 267a4c6..1cacf7f 100644
--- a/lib/librte_eal/common/malloc_heap.c
+++ b/lib/librte_eal/common/malloc_heap.c
@@ -213,7 +213,9 @@
 {
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	unsigned ms_cnt;
-	struct rte_memseg *ms;
+	struct rte_memseg *ms, *prev_ms = NULL;
+	struct malloc_elem *elem, *prev_elem;
+	int ret;
 
 	if (mcfg == NULL)
 		return -1;
@@ -222,6 +224,32 @@
 			(ms_cnt < RTE_MAX_MEMSEG) && (ms->len > 0);
 			ms_cnt++, ms++) {
 		malloc_heap_add_memseg(&mcfg->malloc_heaps[ms->socket_id], ms);
+		elem = (struct malloc_elem *)ms->addr;
+		if (prev_ms != NULL && \
+			(ms->socket_id == prev_ms->socket_id)) {
+			prev_elem = (struct malloc_elem *)prev_ms->addr;
+
+			/* prev_elem and elem to be contiguous for the resize.
+			   Other wise look for prev_elem in iterations */
+			if (elem != RTE_PTR_ADD(prev_elem,
+				prev_elem->size + MALLOC_ELEM_OVERHEAD)) {
+				prev_ms = ms;
+				continue;
+			}
+			/* end BUSY elem pointed by prev_elem can be merged
+			   with prev_elem itself, as it expands it size now.
+			 */
+			prev_elem->size += MALLOC_ELEM_OVERHEAD;
+
+			/* preserve end BUSY elem that points to current elem,
+			   or else free_list will be broken */
+			ret = malloc_elem_resize(prev_elem,
+				prev_elem->size + elem->size - MALLOC_ELEM_OVERHEAD);
+			if (ret < 0)
+				prev_elem = elem;
+		} else {
+			prev_ms = ms;
+		}
 	}
 
 	return 0;
-- 
1.9.1

Re: [dpdk-stable] [PATCH 1/2] eal/malloc: merge malloc_elems in heap if they are contiguous

[Burakov, Anatoly]

On 03-May-18 11:11 AM, Gowrishankar wrote:
> From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
> 
> During malloc heap init, if there are malloc_elems contiguous in
> virt addresses, they could be merged so that, merged malloc_elem
> would guarantee larger free memory size than its actual hugepage
> size, it was created for.
> 
> Fixes: fafcc11985 ("mem: rework memzone to be allocated by malloc")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
> ---

Hi Gowrishankar,

I haven't looked at the patchset in detail yet, however i have a general 
question: how do we end up with VA-contiguous memsegs that are not part 
of the same memseg in the first place? Is there something wrong with 
memseg sorting code? Alternatively, if they were broken up, presumably 
they were broken up for a reason, namely while they may be VA 
contiguous, they weren't IOVA-contiguous.

Can you provide a dump of physmem layout where memory would have been VA 
and IOVA-contiguous while belonging to different memsegs?

-- 
Thanks,
Anatoly

Re: [dpdk-stable] [PATCH 1/2] eal/malloc: merge malloc_elems in heap if they are contiguous

[gowrishankar muthukrishnan]

Hi Anatoly,

On Friday 04 May 2018 02:59 PM, Burakov, Anatoly wrote:
> On 03-May-18 11:11 AM, Gowrishankar wrote:
>> From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
>>
>> During malloc heap init, if there are malloc_elems contiguous in
>> virt addresses, they could be merged so that, merged malloc_elem
>> would guarantee larger free memory size than its actual hugepage
>> size, it was created for.
>>
>> Fixes: fafcc11985 ("mem: rework memzone to be allocated by malloc")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Gowrishankar Muthukrishnan 
>> <gowrishankar.m@linux.vnet.ibm.com>
>> ---
>
> Hi Gowrishankar,
>
> I haven't looked at the patchset in detail yet, however i have a 
> general question: how do we end up with VA-contiguous memsegs that are 
> not part of the same memseg in the first place? Is there something 
> wrong with memseg sorting code? Alternatively, if 
> they were broken up, presumably they were broken up for a reason, 
> namely while they may be VA contiguous, they weren't IOVA-contiguous.

In powerpc, when *nr_overcommit_hugepages set* (to respect address hint 
in get_virtual_area() as requested by secondary process), mmap() would 
not be allocate one big VA chunk for all the available hugepages. In 
order to support secondary process be in same VA
range, we need to add anonymous and hugetlb flags in mmap calls while 
remapping. As mmap can only create max VA at the size of hugepage 
(MAP_HUGETLB) and also to respect address hint (MAP_ANONYMOUS), multiple 
VA chunks are created, even though both VA and IOVA are contiguous in 
most of the cases.

>
> Can you provide a dump of physmem layout where memory would have been 
> VA and IOVA-contiguous while belonging to different memsegs?

Please find here: https://pastebin.com/tDNEaxdU

As you notice malloc_heaps, its index for heap size is 8 which is 
supposedly 11.

To note, these are not problems with memory rework done in latest code 
base. So, I refered code until v18.02.

-- 
Regards,
Gowrishankar M

Re: [dpdk-stable] [PATCH 1/2] eal/malloc: merge malloc_elems in heap if they are contiguous

[Burakov, Anatoly]

On 04-May-18 11:41 AM, gowrishankar muthukrishnan wrote:
> Hi Anatoly,
> 
> On Friday 04 May 2018 02:59 PM, Burakov, Anatoly wrote:
>> On 03-May-18 11:11 AM, Gowrishankar wrote:
>>> From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
>>>
>>> During malloc heap init, if there are malloc_elems contiguous in
>>> virt addresses, they could be merged so that, merged malloc_elem
>>> would guarantee larger free memory size than its actual hugepage
>>> size, it was created for.
>>>
>>> Fixes: fafcc11985 ("mem: rework memzone to be allocated by malloc")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Gowrishankar Muthukrishnan 
>>> <gowrishankar.m@linux.vnet.ibm.com>
>>> ---
>>
>> Hi Gowrishankar,
>>
>> I haven't looked at the patchset in detail yet, however i have a 
>> general question: how do we end up with VA-contiguous memsegs that are 
>> not part of the same memseg in the first place? Is there something 
>> wrong with memseg sorting code? Alternatively, if they were broken up, 
>> presumably they were broken up for a reason, namely while they may be 
>> VA contiguous, they weren't IOVA-contiguous.
> 
> In powerpc, when *nr_overcommit_hugepages set* (to respect address hint 
> in get_virtual_area() as requested by secondary process), mmap() would 
> not be allocate one big VA chunk for all the available hugepages. In 
> order to support secondary process be in same VA
> range, we need to add anonymous and hugetlb flags in mmap calls while 
> remapping. As mmap can only create max VA at the size of hugepage 
> (MAP_HUGETLB) and also to respect address hint (MAP_ANONYMOUS), multiple 
> VA chunks are created, even though both VA and IOVA are contiguous in 
> most of the cases.

OK, suppose on PPC64, that may happen. Still (and please correct me if 
i'm misunderstanding the patchset - as i said, i haven't looked at it in 
detail, and have only taken a cursory look), there are two issues i see 
here:

1) there's no check for IOVA-contiguousness, only VA-contiguousness, 
which means you are risking accidentally concatenating segments that 
aren't IOVA-contiguous. Prior to 18.05, the rest of DPDK expects all 
segments to be VA- and IOVA-contiguous.

2) i don't think this problem should be solved in malloc. Malloc 
elements have memseg pointers in them, and if you concatenate multiple 
segments, you will end up having malloc elements which point to wrong 
segments. Instead, you should fix memseg allocation code to do 
concatenate seemingly disparate segments, and avoid the problem with 
malloc elements in the first place. Maybe do another sorting pass, or 
something. In any case, memseg allocation code is the correct place to 
fix this, IMO.

> 
>>
>> Can you provide a dump of physmem layout where memory would have been 
>> VA and IOVA-contiguous while belonging to different memsegs?
> 
> Please find here: https://pastebin.com/tDNEaxdU
> 
> As you notice malloc_heaps, its index for heap size is 8 which is 
> supposedly 11.

That's a bit hard to read. There's a rte_eal_dump_physmem_layout() 
function that should help display this in a more user-friendly manner :)

> 
> To note, these are not problems with memory rework done in latest code 
> base. So, I refered code until v18.02.
> 


-- 
Thanks,
Anatoly

[dpdk-stable] [PATCH 2/2] eal/malloc: fix heap index to correctly insert memseg

[Gowrishankar]

From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

When there are multiple memsegs created and adding new memseg would
cause bigger heap size, its index in free_head list should be based
on new size of heap. Currently, only the size of elem is accounted
as in malloc_elem_free_list_insert. As heap total size gets bigger,
list of those memsegs should be at the right index, so that
malloc_heap_alloc would find suitable element for the requested
memory size by applications.

Fixes: b0489e7bca ("malloc: fix linear complexity")
Cc: stable@dpdk.org

Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

---
Eg. Below heap is in one numa socket, for the size of 1G (i.e 64*16MB).
All corresponding malloc_elem are always added in heap index 8,
as their size is always 16MB (and due to which, index is also 8 always).

        free_head = {{lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0x0}, {
          lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0x0}, {lh_first = 0x0}, {
          lh_first = 0x0}, {lh_first = 0x7efd3f000000}, {lh_first = 0x0}, {lh_first = 0x0}, {
          lh_first = 0x0}, {lh_first = 0x0}},
        alloc_count = 6, total_size = 1073733632},

Ideally, this list of memsegs should ideally be at slot 12, as they grow heap for 1G.
---
 lib/librte_eal/common/malloc_heap.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
index 1cacf7f..f686e5e 100644
--- a/lib/librte_eal/common/malloc_heap.c
+++ b/lib/librte_eal/common/malloc_heap.c
@@ -105,10 +105,32 @@
 			ms->len - MALLOC_ELEM_OVERHEAD);
 	end_elem = RTE_PTR_ALIGN_FLOOR(end_elem, RTE_CACHE_LINE_SIZE);
 	const size_t elem_size = (uintptr_t)end_elem - (uintptr_t)start_elem;
+	size_t cur_idx, new_idx, heap_size;
 
 	malloc_elem_init(start_elem, heap, ms, elem_size);
 	malloc_elem_mkend(end_elem, start_elem);
+
+	/* Compare heap index based on its current size as well as
+	 * its new size with memseg added. If new size needs new index
+	 * move its free_head to the new slot.
+	 */
+	cur_idx = malloc_elem_free_list_index(heap->total_size);
+	heap_size = heap->total_size + elem_size;
+	new_idx = malloc_elem_free_list_index(heap_size);
+	if (cur_idx != new_idx) {
+		heap->free_head[new_idx] = heap->free_head[cur_idx];
+		memset(&heap->free_head[cur_idx],
+			0, sizeof(heap->free_head[cur_idx]));
+	}
+
+	/* malloc_elem_free_list_insert calculates index based on
+	 * elem->size, hence we set elem->size as new heap size,
+	 * while inserting this elem. After that, we reset elem->size
+	 * to its original value. A minor hack though!.
+	 */
+	start_elem->size = heap_size;
 	malloc_elem_free_list_insert(start_elem);
+	start_elem->size = elem_size;
 
 	heap->total_size += elem_size;
 }
-- 
1.9.1