RE: [EXTERNAL] Re: [PATCH 4/4] net/netvsc: cache device parameters for hot plug events

[Long Li <longli@microsoft.com>]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, January 28, 2025 4:32 PM
> To: Long Li <longli@microsoft.com>
> Cc: longli@linuxonhyperv.com; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Wei Hu <weh@microsoft.com>;
> dev@dpdk.org
> Subject: Re: [EXTERNAL] Re: [PATCH 4/4] net/netvsc: cache device parameters
> for hot plug events
> 
> On Wed, 29 Jan 2025 00:10:12 +0000
> Long Li <longli@microsoft.com> wrote:
> 
> > > Subject: [EXTERNAL] Re: [PATCH 4/4] net/netvsc: cache device
> > > parameters for hot plug events
> > >
> > > On Mon, 27 Jan 2025 17:35:06 -0800
> > > longli@linuxonhyperv.com wrote:
> > >
> > > > @@ -1409,9 +1424,6 @@ eth_hn_dev_uninit(struct rte_eth_dev *eth_dev)
> > > >  	ret_stop = hn_dev_stop(eth_dev);
> > > >  	hn_dev_close(eth_dev);
> > > >
> > > > -	free(hv->vf_devargs);
> > > > -	hv->vf_devargs = NULL;
> > > > -
> > > >  	hn_detach(hv);
> > > >  	hn_chim_uninit(eth_dev);
> > > >  	rte_vmbus_chan_close(hv->channels[0]);
> > > > @@ -1423,6 +1435,61 @@ eth_hn_dev_uninit(struct rte_eth_dev
> *eth_dev)
> > > >  	return ret_stop;
> > > >  }
> > > >
> > > > +static int populate_cache_list(void) {
> > > > +	int ret;
> > > > +	struct rte_devargs *da;
> > > > +
> > > > +	rte_spinlock_lock(&netvsc_lock);
> > > > +	da_cache_usage++;
> > > > +	if (da_cache_usage > 1) {
> > > > +		ret = 0;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	LIST_INIT(&da_cache_list);
> > > > +	RTE_EAL_DEVARGS_FOREACH("pci", da) {
> > > > +		struct da_cache *cache;
> > > > +
> > > > +		cache = rte_zmalloc("NETVSC-HOTADD", sizeof(*cache),
> > > rte_mem_page_size());
> > > > +		if (!cache) {
> > > > +			ret = -ENOMEM;
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		strncpy(cache->name, da->name, sizeof(da->name));
> > > > +		cache->drv_str = strdup(da->drv_str);
> > > > +		if (!cache->drv_str) {
> > > > +			rte_free(cache);
> > > > +			ret = -ENOMEM;
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		LIST_INSERT_HEAD(&da_cache_list, cache, list);
> > > > +	}
> > >
> > > Why do you need to cache entry to be page aligned, that seems
> > > unnecessary wasteful?
> >
> > You are right it doesn't need to. I'm removing the alignment.
> >
> > > Why does it need to be huge pages? versus normal malloc?
> >
> > I thought it would make it easier to debug memory leaks through EAL. But
> normal malloc is fine because this data is not shared between primary/secondary.
> >
> > > The string is coming from malloc (strdup) so it can't be used by secondary
> process.
> > >
> > > Since you are allocating a devargs cache entry, you could just as
> > > well use flexible array where entry was like:
> > > struct da_cache {
> > > 	LIST_ENTRY(da_cache) list;
> > > 	char name[RTE_DEV_NAME_MAX_LEN];
> > > 	char drv_str[];
> > > };
> >
> > This is difficult as I don't know how big it is for drv_str[].
> 
> That is why the malloc adds extra space for drv_str, see below.
> Flexible array allow one array at end of structure
> > >
> > >
> > > 	cache = malloc(sizeof(*cache) + strlen(da->drv_str) + 1); ...
> > > 	strcpy(cache->drv_str, da->drv_str);
> >

Thank you, got it.