Messages in this thread | | | Date | Wed, 11 Dec 2019 09:58:54 -0800 | From | Jakub Kicinski <> | 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?
> 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.
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?
> @@ -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; > };
| |