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

["Lavigne, Jamie" <lavignen@amazon.com>]

Hi Sergio,

> 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 

Updated.

>  
> > 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? 

I like this.  It looks equivalent to my solution, but simpler.  I will post an updated patch.

Jamie