On Wed, 29 Jan 2025 00:10:12 +0000
Long Li <lon...@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
> > lon...@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);
>
> Another approach is to modify EAL to never delete driver arguments when a
> device is removed. i.e., It doesn't call rte_devargs_remove() on device
> removal, instead keep those devargs for the lifetime of the process. Do you
> think this is a better approach? This will save work if other drivers want to
> cache devargs list for device hot plug events.
Not sure, right now I don't think it is possible to pre-add devargs for future
hot plug
