Re: [dpdk-dev] [PATCH v2] Correctly handle malloc_elem resize with padding

[Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>]

Hi Jamie,

On 31/05/2017 01:16, Jamie Lavigne wrote:
> Currently when a malloc_elem is split after resizing, any padding
> present in the elem is ignored.  This causes the resized elem to be too
> small when padding is present, and user data can overwrite the beginning
> of the following malloc_elem.
>
> Solve this by including the size of the padding when computing where to
> split the malloc_elem.

Nice catch!

Could you please rework commit format a bit:
- Add 'mem:' as prefix in your patch title
- I would mention in the title that this is a fix
- Provide 'Fixes' line in commit message

> Signed-off-by: Jamie Lavigne <lavignen@amazon.com>
> ---
>   lib/librte_eal/common/malloc_elem.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/common/malloc_elem.c b/lib/librte_eal/common/malloc_elem.c
> index 42568e1..8766fa8 100644
> --- a/lib/librte_eal/common/malloc_elem.c
> +++ b/lib/librte_eal/common/malloc_elem.c
> @@ -333,9 +333,11 @@ malloc_elem_resize(struct malloc_elem *elem, size_t size)
>   	elem_free_list_remove(next);
>   	join_elem(elem, next);
>   
> -	if (elem->size - new_size >= MIN_DATA_SIZE + MALLOC_ELEM_OVERHEAD){
> +	const size_t new_total_size = new_size + elem->pad;
> +
> +	if (elem->size - new_total_size >= MIN_DATA_SIZE + MALLOC_ELEM_OVERHEAD) {
>   		/* now we have a big block together. Lets cut it down a bit, by splitting */
> -		struct malloc_elem *split_pt = RTE_PTR_ADD(elem, new_size);
> +		struct malloc_elem *split_pt = RTE_PTR_ADD(elem, new_total_size);
>   		split_pt = RTE_PTR_ALIGN_CEIL(split_pt, RTE_CACHE_LINE_SIZE);
>   		split_elem(elem, split_pt);
>   		malloc_elem_free_list_insert(split_pt);

This indeed fixes the issue you have mentioned. I was thinking of the 
following fix instead:
- Add elem->pad to new_size
- Remove current_size var and instead use elem->size

I think those changes should have the same result while removing a 
couple of vars from the function, which I hope would be easier to read.

What do you think?

Thanks,
Sergio