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

Jakub Kicinski
2019-12-11T17:59:00Z

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;
> };