[dpdk-dev] [RFC 4/5] virtio/container: adjust memory initialization process

[Tan, Jianfeng]

> ......
> > > > +int
> > > > +rte_memseg_info_get(int index, int *pfd, uint64_t *psize, void
> > > > +**paddr) {
> > > > +	struct rte_mem_config *mcfg;
> > > > +	mcfg = rte_eal_get_configuration()->mem_config;
> > > > +
> > > > +	*pfd = mcfg->memseg[index].fd;
> > > > +	*psize = (uint64_t)mcfg->memseg[index].len;
> > > > +	*paddr = (void *)(uint64_t)mcfg->memseg[index].addr;
> > > > +	return 0;
> > > > +}
> > >
> > > Wonder who will use that function?
> > > Can't see any references to that function in that patch or next.
> >
> > This function is used in 1/5, when virtio front end needs to send
> VHOST_USER_SET_MEM_TABLE to back end.
> 
> Ok, but hen this function should be defined in the patch *before* it is used,
> not after.
> Another thing: probably better to create a struct for all memseg parameters
> you want to retrieve, and pass it to the function, instead of several pointers.

Very good suggestion! I'll fix it in next version.

> > > > +		addr = rte_eal_shm_create(&fd);
> > >
> > > Why do you remove ability to map(dev/zero) here?
> > > Probably not everyone plan to use --no-hugepages only inside containers.
> >
> > From my understanding, mmap here is just to allocate some memory,
> > which is initialized to be all zero. I cannot understand what's the
> relationship with /dev/zero.
> 
> I used it here as a synonym for mmap(, ..., MAP_ANONYMOUS,...).
> 
>  rte_eal_shm_create() can do the original function, plus it generates a fd to
> point to this chunk of
> > memory. This fd is indispensable in vhost protocol when
> VHOST_USER_SET_MEM_TABLE using sendmsg().
> 
> 
> My question was:
> Right now for --no-hugepages it allocates a chunk of memory that is not
> backed-up by any file and is private to the process:
> 
> addr = mmap(NULL, internal_config.memory, PROT_READ | PROT_WRITE,
> MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> 
> You changed it to shared memory region allocation:
> 
> fd = shm_open(filepath, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR); addr =
> mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> 
> I understand that you need it for your containers stuff - but I suppose you
> have to add new functionality without breaking existing one> There could be
> other users of --no-hugepages and they probably want existing behaviour.
> Konstantin

Thank you for patient analysis and I agree with you. I should have not break
compatibility with existing applications. I'd like to redesign this in next version.
Maybe a new cmd option is necessary here.

Jianfeng

.....
> > > > --
> > > > 2.1.4