Re: [dpdk-dev] [PATCH] eal: unmap unneed dpdk VA spaces for legacy mem

["Lilijun (Jerry, Cloud Networking)" <jerry.lilijun@huawei.com>]

Hi Anatoly,

> -----Original Message-----
> From: Burakov, Anatoly [mailto:anatoly.burakov@intel.com]
> Sent: Friday, March 08, 2019 5:38 PM
> To: Lilijun (Jerry, Cloud Networking) <jerry.lilijun@huawei.com>;
> dev@dpdk.org
> Cc: jerry.zhang@intel.com; ian.stokes@intel.com
> Subject: Re: [dpdk-dev] [PATCH] eal: unmap unneed dpdk VA spaces for
> legacy mem
> 
> On 08-Mar-19 5:38 AM, Lilijun wrote:
> > Comparing dpdk VA spaces to dpdk 16.11, the dpdk app process's VA
> spaces increase to above 30G.
> > Here we can unmap the unneed VA spaces in rte_memseg_list.
> >
> > Signed-off-by: Lilijun <jerry.lilijun@huawei.com>
> > ---
> >   lib/librte_eal/linuxapp/eal/eal_memory.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
> > b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > index 32feb41..56abdd2 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > @@ -1626,8 +1626,19 @@ void numa_error(char *where)
> >   		if (msl->base_va == NULL)
> >   			continue;
> >   		/* skip lists where there is at least one page allocated */
> > -		if (msl->memseg_arr.count > 0)
> > +		if (msl->memseg_arr.count > 0) {
> > +			if (internal_config.legacy_mem) {
> > +				struct rte_fbarray *arr = &msl->memseg_arr;
> > +				int idx = rte_fbarray_find_next_free(arr, 0);
> > +
> > +				while (idx >= 0) {
> > +					void *va = (void*)((char*)msl-
> >base_va + idx * msl->page_sz);
> > +					munmap(va, msl->page_sz);
> > +					idx = rte_fbarray_find_next_free(arr,
> idx + 1);
> > +				}
> 
> I am not entirely convinced this change is safe to do. Technically, this space is
> marked as free, so correctly written code should not attempt to access it,
> however it is still potentially dangerous to have memory area that is
> supposed to be allocated (according to data structures'
> parameters), but isn't.
> 
> If you are deallocating the VA space, ideally you should also resize the
> memseg list (as in, change its length), because that leftover memory area is
> no longer valid. However, this then presents us with a mismatch between
> (va_start + len) and (va_start + page_sz * memseg_arr.len), which may
> break things further.

Yes, you're right, here we need resize the memseg length. I will update it if this patch is needed.
> 
> May i ask what is the purpose of this change? I mean, i understand the part
> about unused VA space sitting there, but what is the consequence of that?
> This isn't 32-bit codepath, and in 64-bit there's plenty of address space to go
> around, and this memory doesn't take up any system resources anyway
> because it is read-only anonymous memory, and is therefore backed by zero
> page instead of real pages. So, what's wrong with just leaving it there?

This change will cause a issues:  when dpdk apps crashed, the coredump file will become too large.

Thanks.

> 
> I don't see any advantage of this change, and i see plenty of disadvantages,
> so for now i'm inclined to NACK this particular patch.
> 
> _However_, i should note that if you feel this is very important feature to
> have and would still like to implement it, my advise would be to look at how
> 32-bit code works, and model the 64-bit implementation after that, because
> 32-bit codepath does exactly what you propose, and doesn't leave unused
> address space.
> 
> > +			} >   			continue;
> > +		}
> >   		/* this is an unused list, deallocate it */
> >   		mem_sz = msl->len;
> >   		munmap(msl->base_va, mem_sz);
> >
> 
> 
> --
> Thanks,
> Anatoly