netdev - Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180323204749.52d30129@cakuba.netronome.com>
Date:   Fri, 23 Mar 2018 20:47:49 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        roopa@...ulusnetworks.com, shm@...ulusnetworks.com,
        jiri@...lanox.com, idosch@...lanox.com,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource
 controller via devlink

On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote:
> From: David Ahern <dsahern@...il.com>
> 
> Add devlink support to netdevsim and use it to implement a simple,
> profile based resource controller. Only one controller is needed
> per namespace, so the first netdevsim netdevice in a namespace
> registers with devlink. If that device is deleted, the resource
> settings are deleted.

FWIW some nits from me blow.

> diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
> index 09388c06171d..449b2a1a1800 100644
> --- a/drivers/net/netdevsim/Makefile
> +++ b/drivers/net/netdevsim/Makefile
> @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y)
>  netdevsim-objs += \
>  	bpf.o
>  endif
> +
> +ifneq ($(CONFIG_NET_DEVLINK),)

Hm.  Don't you need MAY_USE_DEVLINK dependency perhaps?

> +netdevsim-objs += devlink.o fib.o
> +endif
> diff --git a/drivers/net/netdevsim/devlink.c b/drivers/net/netdevsim/devlink.c
> new file mode 100644
> index 000000000000..d10558e1b022
> --- /dev/null
> +++ b/drivers/net/netdevsim/devlink.c
 
> +static int devlink_resources_register(struct devlink *devlink)
> +{
> +	struct devlink_resource_size_params params = {
> +		.size_max = (u64)-1,
> +		.size_granularity = 1,
> +		.unit = DEVLINK_RESOURCE_UNIT_ENTRY
> +	};
> +	struct net *net = devlink_net(devlink);
> +	int err;
> +	u64 n;
> +
> +	/* Resources for IPv4 */
> +	err = devlink_resource_register(devlink, "IPv4", (u64)-1,
> +					NSIM_RESOURCE_IPV4,
> +					DEVLINK_RESOURCE_ID_PARENT_TOP,
> +					&params, NULL);
> +	if (err) {
> +		pr_err("Failed to register IPv4 top resource\n");
> +		goto out;

nit: why goto out here and return err everywhere else?  If I was to
     choose I'd rather see returns.  goto X; X: return; is less
     obviously correct IMHO.  Besides labels should be called by the
     action they perform/undo, so goto err_return? :S

> +	}
> +
> +	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, true);
> +	err = devlink_resource_register(devlink, "fib", n,
> +					NSIM_RESOURCE_IPV4_FIB,
> +					NSIM_RESOURCE_IPV4,
> +					&params, &nsim_ipv4_fib_res_ops);
> +	if (err) {
> +		pr_err("Failed to register IPv4 FIB resource\n");
> +		return err;
> +	}
> +
> +	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, true);
> +	err = devlink_resource_register(devlink, "fib-rules", n,
> +					NSIM_RESOURCE_IPV4_FIB_RULES,
> +					NSIM_RESOURCE_IPV4,
> +					&params, &nsim_ipv4_fib_rules_res_ops);
> +	if (err) {
> +		pr_err("Failed to register IPv4 FIB rules resource\n");
> +		return err;
> +	}
> +
> +	/* Resources for IPv6 */
> +	err = devlink_resource_register(devlink, "IPv6", (u64)-1,
> +					NSIM_RESOURCE_IPV6,
> +					DEVLINK_RESOURCE_ID_PARENT_TOP,
> +					&params, NULL);
> +	if (err) {
> +		pr_err("Failed to register IPv6 top resource\n");
> +		goto out;
> +	}
> +
> +	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, true);
> +	err = devlink_resource_register(devlink, "fib", n,
> +					NSIM_RESOURCE_IPV6_FIB,
> +					NSIM_RESOURCE_IPV6,
> +					&params, &nsim_ipv6_fib_res_ops);
> +	if (err) {
> +		pr_err("Failed to register IPv6 FIB resource\n");
> +		return err;
> +	}
> +
> +	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, true);
> +	err = devlink_resource_register(devlink, "fib-rules", n,
> +					NSIM_RESOURCE_IPV6_FIB_RULES,
> +					NSIM_RESOURCE_IPV6,
> +					&params, &nsim_ipv6_fib_rules_res_ops);
> +	if (err) {
> +		pr_err("Failed to register IPv6 FIB rules resource\n");
> +		return err;
> +	}
> +out:
> +	return err;
> +}

> +void nsim_devlink_teardown(struct netdevsim *ns)
> +{
> +	if (ns->devlink) {
> +		struct net *net = dev_net(ns->netdev);
> +		bool *reg_devlink = net_generic(net, nsim_devlink_id);

nit: reverse xmas tree

> +		devlink_unregister(ns->devlink);
> +		devlink_free(ns->devlink);
> +		ns->devlink = NULL;
> +
> +		nsim_devlink_net_reset(net);
> +		*reg_devlink = true;
> +	}
> +}

> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
> new file mode 100644
> index 000000000000..b77dcafc7158
> --- /dev/null
> +++ b/drivers/net/netdevsim/fib.c

> +static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event,
> +			     void *ptr)
> +{
> +	struct fib_notifier_info *info = ptr;
> +	int err;
> +
> +	switch (event) {
> +	case FIB_EVENT_RULE_ADD: /* fall through */

nit: I don't think fall through comment is needed for back-to-back
     cases.

> +	case FIB_EVENT_RULE_DEL:
> +		err = nsim_fib_rule_event(info, event == FIB_EVENT_RULE_ADD);
> +		break;
> +
> +	case FIB_EVENT_ENTRY_ADD:  /* fall through */
> +	case FIB_EVENT_ENTRY_DEL:
> +		err = nsim_fib_event(info, event == FIB_EVENT_ENTRY_ADD);
> +		break;
> +	}
> +
> +	return notifier_from_errno(err);
> +}