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

[Yuval Avnery]

> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
> Sent: Wednesday, December 11, 2019 9:59 AM
> To: Yuval Avnery <yuvalav@xxxxxxxxxxxx>
> Cc: Jiri Pirko <jiri@xxxxxxxxxxxx>; davem@xxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
> 
> 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".

> 
> > Signed-off-by: Yuval Avnery <yuvalav@xxxxxxxxxxxx>
> > Acked-by: Jiri Pirko <jiri@xxxxxxxxxxxx>
> 
> > 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?

> 
> 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
> 
> >  	nsim_bus_dev->num_vfs = num_vfs;
> > @@ -38,8 +38,6 @@ static int nsim_bus_dev_vfs_enable(struct
> > nsim_bus_dev *nsim_bus_dev,
> >
> >  static void nsim_bus_dev_vfs_disable(struct nsim_bus_dev
> > *nsim_bus_dev)  {
> > -	kfree(nsim_bus_dev->vfconfigs);
> > -	nsim_bus_dev->vfconfigs = NULL;
> >  	nsim_bus_dev->num_vfs = 0;
> >  }
> >
> > @@ -154,22 +152,29 @@ static struct device_type nsim_bus_dev_type = {
> > };
> >
> >  static struct nsim_bus_dev *
> > -nsim_bus_dev_new(unsigned int id, unsigned int port_count);
> > +nsim_bus_dev_new(unsigned int id, unsigned int port_count,
> > +		 unsigned int max_vfs);
> > +
> > +#define NSIM_BUS_DEV_MAX_VFS 4
> >
> >  static ssize_t
> >  new_device_store(struct bus_type *bus, const char *buf, size_t count)
> > {
> >  	struct nsim_bus_dev *nsim_bus_dev;
> >  	unsigned int port_count;
> > +	unsigned int max_vfs;
> >  	unsigned int id;
> >  	int err;
> >
> > -	err = sscanf(buf, "%u %u", &id, &port_count);
> > +	err = sscanf(buf, "%u %u %u", &id, &port_count, &max_vfs);
> >  	switch (err) {
> >  	case 1:
> >  		port_count = 1;
> >  		/* fall through */
> >  	case 2:
> > +		max_vfs = NSIM_BUS_DEV_MAX_VFS;
> > +		/* fall through */
> > +	case 3:
> >  		if (id > INT_MAX) {
> >  			pr_err("Value of \"id\" is too big.\n");
> >  			return -EINVAL;
> 
> Is 0 VFs okay? will kcalloc(0, size, flags) behave correctly?

Right, I will fix.

> 
> > @@ -179,7 +184,7 @@ new_device_store(struct bus_type *bus, const char
> *buf, size_t count)
> >  		pr_err("Format for adding new device is \"id port_count\"
> (uint uint).\n");
> >  		return -EINVAL;
> >  	}
> > -	nsim_bus_dev = nsim_bus_dev_new(id, port_count);
> > +	nsim_bus_dev = nsim_bus_dev_new(id, port_count, max_vfs);
> >  	if (IS_ERR(nsim_bus_dev))
> >  		return PTR_ERR(nsim_bus_dev);
> >
> > @@ -267,7 +272,8 @@ static struct bus_type nsim_bus = {  };
> >
> >  static struct nsim_bus_dev *
> > -nsim_bus_dev_new(unsigned int id, unsigned int port_count)
> > +nsim_bus_dev_new(unsigned int id, unsigned int port_count,
> > +		 unsigned int max_vfs)
> >  {
> >  	struct nsim_bus_dev *nsim_bus_dev;
> >  	int err;
> > @@ -284,12 +290,24 @@ nsim_bus_dev_new(unsigned int id, unsigned int
> port_count)
> >  	nsim_bus_dev->dev.type = &nsim_bus_dev_type;
> >  	nsim_bus_dev->port_count = port_count;
> >  	nsim_bus_dev->initial_net = current->nsproxy->net_ns;
> > +	nsim_bus_dev->max_vfs = max_vfs;
> > +
> > +	nsim_bus_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
> > +					  sizeof(struct nsim_vf_config),
> > +					  GFP_KERNEL);
> > +	if (!nsim_bus_dev->vfconfigs) {
> > +		err = -ENOMEM;
> > +		goto err_nsim_bus_dev_id_free;
> > +	}
> >
> >  	err = device_register(&nsim_bus_dev->dev);
> >  	if (err)
> > -		goto err_nsim_bus_dev_id_free;
> > +		goto err_nsim_vfconfigs_free;
> > +
> >  	return nsim_bus_dev;
> >
> > +err_nsim_vfconfigs_free:
> > +	kfree(nsim_bus_dev->vfconfigs);
> >  err_nsim_bus_dev_id_free:
> >  	ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
> >  err_nsim_bus_dev_free:
> > @@ -301,6 +319,7 @@ static void nsim_bus_dev_del(struct nsim_bus_dev
> > *nsim_bus_dev)  {
> >  	device_unregister(&nsim_bus_dev->dev);
> >  	ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
> > +	kfree(nsim_bus_dev->vfconfigs);
> >  	kfree(nsim_bus_dev);
> >  }
> >
> > diff --git a/drivers/net/netdevsim/netdevsim.h
> > b/drivers/net/netdevsim/netdevsim.h
> > index 94df795ef4d3..e2049856add8 100644
> > --- a/drivers/net/netdevsim/netdevsim.h
> > +++ b/drivers/net/netdevsim/netdevsim.h
> > @@ -238,6 +238,7 @@ struct nsim_bus_dev {
> >  	struct net *initial_net; /* Purpose of this is to carry net pointer
> >  				  * during the probe time only.
> >  				  */
> > +	unsigned int max_vfs;
> >  	unsigned int num_vfs;
> >  	struct nsim_vf_config *vfconfigs;
> >  };