On 3/23/18 9:47 PM, Jakub Kicinski wrote:

On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote:

quoted

From: David Ahern <redacted>

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.

quoted

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?

mlxsw uses CONFIG_NET_DEVLINK in its Makefile.

MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me
why it is needed at all.

quoted

+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

quoted

+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

Will fix. Just got lost in the many iterations leading up to the RFC.

quoted

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

quoted

+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

reg_devlink uses net, so the order can not be changed

quoted

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

quoted

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

quoted

+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.

ok.

Thanks for the review.

quoted

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