Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Wed, 11 Dec 2019 18:19:56 +0000, Yuval Avnery wrote:
> > On Wed, 11 Dec 2019 04:58:53 +0200, Yuval Avnery wrote:  
> > > Currently there is no limit to the number of VFs netdevsim can enable.
> > > In a real systems this value exist and used by driver.
> > > Fore example, Some features might need to consider this value when
> > > allocating memory.  
> > 
> > Thanks for the patch!
> > 
> > Can you shed a little bit more light on where it pops up? Just for my curiosity?  
> 
> Yes, like we described in the subdev threads.
> User should be able to configure some attributes before the VF was enabled.
> So all those (persistent) VF attributes should be available for query and configuration
> before VF was enabled.
> The driver can allocate an array according to max_vfs to hold all that data,
> like we do here in" vfconfigs".

I was after more practical reasoning, are you writing some tests for
subdev stuff that will depend on this change? :)

> > > Signed-off-by: Yuval Avnery <yuvalav@mellanox.com>
> > > Acked-by: Jiri Pirko <jiri@mellanox.com>  
> > >
> > > diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> > > index 6aeed0c600f8..f1a0171080cb 100644
> > > --- a/drivers/net/netdevsim/bus.c
> > > +++ b/drivers/net/netdevsim/bus.c
> > > @@ -26,9 +26,9 @@ static struct nsim_bus_dev *to_nsim_bus_dev(struct
> > > device *dev)  static int nsim_bus_dev_vfs_enable(struct nsim_bus_dev  
> > *nsim_bus_dev,  
> > >  				   unsigned int num_vfs)
> > >  {
> > > -	nsim_bus_dev->vfconfigs = kcalloc(num_vfs,
> > > -					  sizeof(struct nsim_vf_config),
> > > -					  GFP_KERNEL);  
> > 
> > You're changing the semantics of the enable/disable as well now.
> > The old values used to be wiped when SR-IOV is disabled, now they will be
> > retained across disable/enable pair.
> > 
> > I think it'd be better if that wasn't the case. Users may expect a system to be
> > in the same state after they enable SR-IOV, regardless if someone else used
> > SR-IOV since last reboot.  
> 
> Right, 
> But some values should retain across enable/disable, for example MAC address which is persistent.
> So maybe we need to retain some values, while resetting others on disable?
> Would that work?

Mmm. That is a good question. For all practical purposes SR-IOV used 
to be local to the host that enables it until Smart/middle box NICs
emerged.

Perhaps the best way forward would be to reset the config that was set
via legacy APIs and keep only the MACs provisioned via persistent
devlink API?

So for now we'd memset, and once devlink API lands reset selectively?

> > Could you add a memset(,0,) here?
> >   
> > > +	if (nsim_bus_dev->max_vfs < num_vfs)
> > > +		return -ENOMEM;
> > > +
> > >  	if (!nsim_bus_dev->vfconfigs)
> > >  		return -ENOMEM;  
> > 
> > This check seems useless now, no? We will always have vfconfigs