[PATCH net-next 0/6] net: Allow FIB notifiers to fail add and replace

[PATCH net-next 0/6] net: Allow FIB notifiers to fail add and replace

[David Ahern]

I wanted to revisit how resource overload is handled for hardware offload
of FIB entries and rules. At the moment, the in-kernel fib notifier can
tell a driver about a route or rule add, replace, and delete, but the
notifier can not affect the action. Specifically, in the case of mlxsw
if a route or rule add is going to overflow the ASIC resources the only
recourse is to abort hardware offload. Aborting offload is akin to taking
down the switch as the path from data plane to the control plane simply
can not support the traffic bandwidth of the front panel ports. Further,
the current state of FIB notifiers is inconsistent with other resources
where a driver can affect a user request - e.g., enslavement of a port
into a bridge or a VRF.

As a result of the work done over the past 3+ years, I believe we are
at a point where we can bring consistency to the stack and offloads,
and reliably allow the FIB notifiers to fail a request, pushing an error
along with a suitable error message back to the user. Rather than
aborting offload when the switch is out of resources, userspace is simply
prevented from adding more routes and has a clear indication of why.

This set does not resolve the corner case where rules or routes not
supported by the device are installed prior to the driver getting loaded
and registering for FIB notifications. In that case, hardware offload has
not been established and it can refuse to offload anything, sending
errors back to userspace via extack. Since conceptually the driver owns
the netdevices associated with its asic, this corner case mainly applies
to unsupported rules and any races during the bringup phase.

Patch 1 fixes call_fib_notifiers to extract the errno from the encoded
response from handlers.

Patches 2-5 allow the call to call_fib_notifiers to fail the add or
replace of a route or rule.

Patch 6 adds a simple resource controller to netdevsim to illustrate
how a FIB resource controller can limit the number of route entries.

Changes since RFC
- correct return code for call_fib_notifier
- dropped patch 6 exporting devlink symbols
- limited example resource controller to init_net only
- updated Kconfig for netdevsim to use MAY_USE_DEVLINK
- updated cover letter regarding startup case noted by Ido

David Ahern (6):
  net: Fix fib notifer to return errno
  net: Move call_fib_rule_notifiers up in fib_nl_newrule
  net/ipv4: Move call_fib_entry_notifiers up for new routes
  net/ipv4: Allow notifier to fail route replace
  net/ipv6: Move call_fib6_entry_notifiers up for route adds
  netdevsim: Add simple FIB resource controller via devlink

 drivers/net/Kconfig               |   1 +
 drivers/net/netdevsim/Makefile    |   4 +
 drivers/net/netdevsim/devlink.c   | 294 ++++++++++++++++++++++++++++++++++++++
 drivers/net/netdevsim/fib.c       | 263 ++++++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdev.c    |  12 +-
 drivers/net/netdevsim/netdevsim.h |  43 ++++++
 net/core/fib_notifier.c           |  10 +-
 net/core/fib_rules.c              |   6 +-
 net/ipv4/fib_trie.c               |  27 +++-
 net/ipv6/ip6_fib.c                |  16 ++-
 10 files changed, 664 insertions(+), 12 deletions(-)
 create mode 100644 drivers/net/netdevsim/devlink.c
 create mode 100644 drivers/net/netdevsim/fib.c

-- 
2.11.0

[PATCH net-next 1/6] net: Fix fib notifer to return errno

[David Ahern]

Notifier handlers use notifier_from_errno to convert any potential error
to an encoded format. As a consequence the other side, call_fib_notifier{s}
in this case, needs to use notifier_to_errno to return the error from
the handler back to its caller.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/core/fib_notifier.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/core/fib_notifier.c b/net/core/fib_notifier.c
index 0c048bdeb016..b793b523aba3 100644
--- a/net/core/fib_notifier.c
+++ b/net/core/fib_notifier.c
@@ -13,16 +13,22 @@ int call_fib_notifier(struct notifier_block *nb, struct net *net,
 		      enum fib_event_type event_type,
 		      struct fib_notifier_info *info)
 {
+	int err;
+
 	info->net = net;
-	return nb->notifier_call(nb, event_type, info);
+	err = nb->notifier_call(nb, event_type, info);
+	return notifier_to_errno(err);
 }
 EXPORT_SYMBOL(call_fib_notifier);
 
 int call_fib_notifiers(struct net *net, enum fib_event_type event_type,
 		       struct fib_notifier_info *info)
 {
+	int err;
+
 	info->net = net;
-	return atomic_notifier_call_chain(&fib_chain, event_type, info);
+	err = atomic_notifier_call_chain(&fib_chain, event_type, info);
+	return notifier_to_errno(err);
 }
 EXPORT_SYMBOL(call_fib_notifiers);
 
-- 
2.11.0

[PATCH net-next 2/6] net: Move call_fib_rule_notifiers up in fib_nl_newrule

[David Ahern]

Move call_fib_rule_notifiers up in fib_nl_newrule to the point right
before the rule is inserted into the list. At this point there are no
more failure paths within the core rule code, so if the notifier
does not fail then the rule will be inserted into the list.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/core/fib_rules.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index 9d87ce868402..33958f84c173 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -631,6 +631,11 @@ int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		goto errout_free;
 
+	err = call_fib_rule_notifiers(net, FIB_EVENT_RULE_ADD, rule, ops,
+				      extack);
+	if (err < 0)
+		goto errout_free;
+
 	list_for_each_entry(r, &ops->rules_list, list) {
 		if (r->pref > rule->pref)
 			break;
@@ -667,7 +672,6 @@ int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (rule->tun_id)
 		ip_tunnel_need_metadata();
 
-	call_fib_rule_notifiers(net, FIB_EVENT_RULE_ADD, rule, ops, extack);
 	notify_rule_change(RTM_NEWRULE, rule, ops, nlh, NETLINK_CB(skb).portid);
 	flush_route_cache(ops);
 	rules_ops_put(ops);
-- 
2.11.0

[PATCH net-next 3/6] net/ipv4: Move call_fib_entry_notifiers up for new routes

[David Ahern]

Move call to call_fib_entry_notifiers for new IPv4 routes to right
before the call to fib_insert_alias. At this point the only remaining
failure path is memory allocations in fib_insert_node. Handle that
very unlikely failure with a call to call_fib_entry_notifiers to
tell drivers about it.

At this point notifier handlers can decide the fate of the new route
with a clean path to delete the potential new entry if the notifier
returns non-0.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/fib_trie.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index fac0b73e24d1..67116233e2bc 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1065,6 +1065,9 @@ static int fib_insert_node(struct trie *t, struct key_vector *tp,
 	return -ENOMEM;
 }
 
+/* fib notifier for ADD is sent before calling fib_insert_alias with
+ * the expectation that the only possible failure ENOMEM
+ */
 static int fib_insert_alias(struct trie *t, struct key_vector *tp,
 			    struct key_vector *l, struct fib_alias *new,
 			    struct fib_alias *fa, t_key key)
@@ -1263,21 +1266,32 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
 	new_fa->tb_id = tb->tb_id;
 	new_fa->fa_default = -1;
 
+	err = call_fib_entry_notifiers(net, event, key, plen, new_fa, extack);
+	if (err)
+		goto out_free_new_fa;
+
 	/* Insert new entry to the list. */
 	err = fib_insert_alias(t, tp, l, new_fa, fa, key);
 	if (err)
-		goto out_free_new_fa;
+		goto out_fib_notif;
 
 	if (!plen)
 		tb->tb_num_default++;
 
 	rt_cache_flush(cfg->fc_nlinfo.nl_net);
-	call_fib_entry_notifiers(net, event, key, plen, new_fa, extack);
 	rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, new_fa->tb_id,
 		  &cfg->fc_nlinfo, nlflags);
 succeeded:
 	return 0;
 
+out_fib_notif:
+	/* notifier was sent that entry would be added to trie, but
+	 * the add failed and need to recover. Only failure for
+	 * fib_insert_alias is ENOMEM.
+	 */
+	NL_SET_ERR_MSG(extack, "Failed to insert route into trie");
+	call_fib_entry_notifiers(net, FIB_EVENT_ENTRY_DEL, key,
+				 plen, new_fa, NULL);
 out_free_new_fa:
 	kmem_cache_free(fn_alias_kmem, new_fa);
 out:
-- 
2.11.0

[PATCH net-next 4/6] net/ipv4: Allow notifier to fail route replace

[David Ahern]

Add checking to call to call_fib_entry_notifiers for IPv4 route replace.
Allows a notifier handler to fail the replace.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/fib_trie.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 67116233e2bc..3dcffd3ce98c 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1219,8 +1219,13 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
 			new_fa->tb_id = tb->tb_id;
 			new_fa->fa_default = -1;
 
-			call_fib_entry_notifiers(net, FIB_EVENT_ENTRY_REPLACE,
-						 key, plen, new_fa, extack);
+			err = call_fib_entry_notifiers(net,
+						       FIB_EVENT_ENTRY_REPLACE,
+						       key, plen, new_fa,
+						       extack);
+			if (err)
+				goto out_free_new_fa;
+
 			rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen,
 				  tb->tb_id, &cfg->fc_nlinfo, nlflags);
 
-- 
2.11.0

[PATCH net-next 5/6] net/ipv6: Move call_fib6_entry_notifiers up for route adds

[David Ahern]

Move call to call_fib6_entry_notifiers for new IPv6 routes to right
before the insertion into the FIB. At this point notifier handlers can
decide the fate of the new route with a clean path to delete the
potential new entry if the notifier returns non-0.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv6/ip6_fib.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 908b8e5b615a..deab2db6692e 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1007,12 +1007,16 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 		if (err)
 			return err;
 
+		err = call_fib6_entry_notifiers(info->nl_net,
+						FIB_EVENT_ENTRY_ADD,
+						rt, extack);
+		if (err)
+			return err;
+
 		rcu_assign_pointer(rt->rt6_next, iter);
 		atomic_inc(&rt->rt6i_ref);
 		rcu_assign_pointer(rt->rt6i_node, fn);
 		rcu_assign_pointer(*ins, rt);
-		call_fib6_entry_notifiers(info->nl_net, FIB_EVENT_ENTRY_ADD,
-					  rt, extack);
 		if (!info->skip_notify)
 			inet6_rt_notify(RTM_NEWROUTE, rt, info, nlflags);
 		info->nl_net->ipv6.rt6_stats->fib_rt_entries++;
@@ -1036,12 +1040,16 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 		if (err)
 			return err;
 
+		err = call_fib6_entry_notifiers(info->nl_net,
+						FIB_EVENT_ENTRY_REPLACE,
+						rt, extack);
+		if (err)
+			return err;
+
 		atomic_inc(&rt->rt6i_ref);
 		rcu_assign_pointer(rt->rt6i_node, fn);
 		rt->rt6_next = iter->rt6_next;
 		rcu_assign_pointer(*ins, rt);
-		call_fib6_entry_notifiers(info->nl_net, FIB_EVENT_ENTRY_REPLACE,
-					  rt, extack);
 		if (!info->skip_notify)
 			inet6_rt_notify(RTM_NEWROUTE, rt, info, NLM_F_REPLACE);
 		if (!(fn->fn_flags & RTN_RTINFO)) {
-- 
2.11.0

[PATCH net-next 6/6] netdevsim: Add simple FIB resource controller via devlink

[David Ahern]

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.

The resource controller allows a user to limit the number of IPv4 and
IPv6 FIB entries and FIB rules. The resource paths are:
    /IPv4
    /IPv4/fib
    /IPv4/fib-rules
    /IPv6
    /IPv6/fib
    /IPv6/fib-rules

The IPv4 and IPv6 top level resources are unlimited in size and can not
be changed. From there, the number of FIB entries and FIB rule entries
are unlimited by default. A user can specify a limit for the fib and
fib-rules resources:

    $ devlink resource set netdevsim/netdevsim0 path /IPv4/fib size 96
    $ devlink resource set netdevsim/netdevsim0 path /IPv4/fib-rules size 16
    $ devlink resource set netdevsim/netdevsim0 path /IPv6/fib size 64
    $ devlink resource set netdevsim/netdevsim0 path /IPv6/fib-rules size 16
    $ devlink dev reload netdevsim/netdevsim0

such that the number of rules or routes is limited (96 ipv4 routes in the
example above):
    $ for n in $(seq 1 32); do ip ro add 10.99.$n.0/24 dev eth1; done
    Error: netdevsim: Exceeded number of supported fib entries.

    $ devlink resource show netdevsim/netdevsim0
    netdevsim/netdevsim0:
      name IPv4 size unlimited unit entry size_min 0 size_max unlimited size_gran 1 dpipe_tables non
        resources:
          name fib size 96 occ 96 unit entry size_min 0 size_max unlimited size_gran 1 dpipe_tables
    ...

With this template in place for resource management, it is fairly trivial
to extend and shows one way to implement a simple counter based resource
controller typical of network profiles.

Currently, devlink only supports initial namespace. Code is in place to
adapt netdevsim to a per namespace controller once the network namespace
issues are resolved.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 drivers/net/Kconfig               |   1 +
 drivers/net/netdevsim/Makefile    |   4 +
 drivers/net/netdevsim/devlink.c   | 294 ++++++++++++++++++++++++++++++++++++++
 drivers/net/netdevsim/fib.c       | 263 ++++++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdev.c    |  12 +-
 drivers/net/netdevsim/netdevsim.h |  43 ++++++
 6 files changed, 616 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/netdevsim/devlink.c
 create mode 100644 drivers/net/netdevsim/fib.c

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 08b85215c2be..891846655000 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -500,6 +500,7 @@ source "drivers/net/hyperv/Kconfig"
 config NETDEVSIM
 	tristate "Simulated networking device"
 	depends on DEBUG_FS
+	depends on MAY_USE_DEVLINK
 	help
 	  This driver is a developer testing tool and software model that can
 	  be used to test various control path networking APIs, especially
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),)
+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..bbdcf064ba10
--- /dev/null
+++ b/drivers/net/netdevsim/devlink.c
@@ -0,0 +1,294 @@
+/*
+ * Copyright (c) 2018 Cumulus Networks. All rights reserved.
+ * Copyright (c) 2018 David Ahern <dsa@cumulusnetworks.com>
+ *
+ * This software is licensed under the GNU General License Version 2,
+ * June 1991 as shown in the file COPYING in the top-level directory of this
+ * source tree.
+ *
+ * THE COPYRIGHT HOLDERS AND/OR OTHER PARTIES PROVIDE THE PROGRAM "AS IS"
+ * WITHOUT WARRANTY OF ANY KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING,
+ * BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE. THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE
+ * OF THE PROGRAM IS WITH YOU. SHOULD THE PROGRAM PROVE DEFECTIVE, YOU ASSUME
+ * THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.
+ */
+
+#include <linux/device.h>
+#include <net/devlink.h>
+#include <net/netns/generic.h>
+
+#include "netdevsim.h"
+
+static unsigned int nsim_devlink_id;
+
+/* place holder until devlink and namespaces is sorted out */
+static struct net *nsim_devlink_net(struct devlink *devlink)
+{
+	return &init_net;
+}
+
+/* IPv4
+ */
+static u64 nsim_ipv4_fib_resource_occ_get(struct devlink *devlink)
+{
+	struct net *net = nsim_devlink_net(devlink);
+
+	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, false);
+}
+
+static struct devlink_resource_ops nsim_ipv4_fib_res_ops = {
+	.occ_get = nsim_ipv4_fib_resource_occ_get,
+};
+
+static u64 nsim_ipv4_fib_rules_res_occ_get(struct devlink *devlink)
+{
+	struct net *net = nsim_devlink_net(devlink);
+
+	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, false);
+}
+
+static struct devlink_resource_ops nsim_ipv4_fib_rules_res_ops = {
+	.occ_get = nsim_ipv4_fib_rules_res_occ_get,
+};
+
+/* IPv6
+ */
+static u64 nsim_ipv6_fib_resource_occ_get(struct devlink *devlink)
+{
+	struct net *net = nsim_devlink_net(devlink);
+
+	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, false);
+}
+
+static struct devlink_resource_ops nsim_ipv6_fib_res_ops = {
+	.occ_get = nsim_ipv6_fib_resource_occ_get,
+};
+
+static u64 nsim_ipv6_fib_rules_res_occ_get(struct devlink *devlink)
+{
+	struct net *net = nsim_devlink_net(devlink);
+
+	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, false);
+}
+
+static struct devlink_resource_ops nsim_ipv6_fib_rules_res_ops = {
+	.occ_get = nsim_ipv6_fib_rules_res_occ_get,
+};
+
+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 = nsim_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;
+	}
+
+	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;
+}
+
+static int nsim_devlink_reload(struct devlink *devlink)
+{
+	enum nsim_resource_id res_ids[] = {
+		NSIM_RESOURCE_IPV4_FIB, NSIM_RESOURCE_IPV4_FIB_RULES,
+		NSIM_RESOURCE_IPV6_FIB, NSIM_RESOURCE_IPV6_FIB_RULES
+	};
+	struct net *net = nsim_devlink_net(devlink);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(res_ids); ++i) {
+		int err;
+		u64 val;
+
+		err = devlink_resource_size_get(devlink, res_ids[i], &val);
+		if (!err) {
+			err = nsim_fib_set_max(net, res_ids[i], val);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
+static void nsim_devlink_net_reset(struct net *net)
+{
+	enum nsim_resource_id res_ids[] = {
+		NSIM_RESOURCE_IPV4_FIB, NSIM_RESOURCE_IPV4_FIB_RULES,
+		NSIM_RESOURCE_IPV6_FIB, NSIM_RESOURCE_IPV6_FIB_RULES
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(res_ids); ++i) {
+		if (nsim_fib_set_max(net, res_ids[i], (u64)-1)) {
+			pr_err("Failed to reset limit for resource %u\n",
+			       res_ids[i]);
+		}
+	}
+}
+
+static const struct devlink_ops nsim_devlink_ops = {
+	.reload = nsim_devlink_reload,
+};
+
+/* once devlink / namespace issues are sorted out
+ * this needs to be net in which a devlink instance
+ * is to be created. e.g., dev_net(ns->netdev)
+ */
+static struct net *nsim_to_net(struct netdevsim *ns)
+{
+	return &init_net;
+}
+
+void nsim_devlink_teardown(struct netdevsim *ns)
+{
+	if (ns->devlink) {
+		struct net *net = nsim_to_net(ns);
+		bool *reg_devlink = net_generic(net, nsim_devlink_id);
+
+		devlink_unregister(ns->devlink);
+		devlink_free(ns->devlink);
+		ns->devlink = NULL;
+
+		nsim_devlink_net_reset(net);
+		*reg_devlink = true;
+	}
+}
+
+void nsim_devlink_setup(struct netdevsim *ns)
+{
+	struct net *net = nsim_to_net(ns);
+	bool *reg_devlink = net_generic(net, nsim_devlink_id);
+	struct devlink *devlink;
+	int err = -ENOMEM;
+
+	/* only one device per namespace controls devlink */
+	if (!*reg_devlink) {
+		ns->devlink = NULL;
+		return;
+	}
+
+	devlink = devlink_alloc(&nsim_devlink_ops, 0);
+	if (!devlink)
+		return;
+
+	err = devlink_register(devlink, &ns->dev);
+	if (err)
+		goto err_devlink_free;
+
+	err = devlink_resources_register(devlink);
+	if (err)
+		goto err_dl_unregister;
+
+	ns->devlink = devlink;
+
+	*reg_devlink = false;
+
+	return;
+
+err_dl_unregister:
+	devlink_unregister(devlink);
+err_devlink_free:
+	devlink_free(devlink);
+}
+
+/* Initialize per network namespace state */
+static int __net_init nsim_devlink_netns_init(struct net *net)
+{
+	bool *reg_devlink = net_generic(net, nsim_devlink_id);
+
+	*reg_devlink = true;
+
+	return 0;
+}
+
+static struct pernet_operations nsim_devlink_net_ops __net_initdata = {
+	.init = nsim_devlink_netns_init,
+	.id   = &nsim_devlink_id,
+	.size = sizeof(bool),
+};
+
+void nsim_devlink_exit(void)
+{
+	unregister_pernet_subsys(&nsim_devlink_net_ops);
+	nsim_fib_exit();
+}
+
+int nsim_devlink_init(void)
+{
+	int err;
+
+	err = nsim_fib_init();
+	if (err)
+		goto err_out;
+
+	err = register_pernet_subsys(&nsim_devlink_net_ops);
+	if (err)
+		nsim_fib_exit();
+
+err_out:
+	return err;
+}
diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
new file mode 100644
index 000000000000..0d105bafa261
--- /dev/null
+++ b/drivers/net/netdevsim/fib.c
@@ -0,0 +1,263 @@
+/*
+ * Copyright (c) 2018 Cumulus Networks. All rights reserved.
+ * Copyright (c) 2018 David Ahern <dsa@cumulusnetworks.com>
+ *
+ * This software is licensed under the GNU General License Version 2,
+ * June 1991 as shown in the file COPYING in the top-level directory of this
+ * source tree.
+ *
+ * THE COPYRIGHT HOLDERS AND/OR OTHER PARTIES PROVIDE THE PROGRAM "AS IS"
+ * WITHOUT WARRANTY OF ANY KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING,
+ * BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE. THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE
+ * OF THE PROGRAM IS WITH YOU. SHOULD THE PROGRAM PROVE DEFECTIVE, YOU ASSUME
+ * THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.
+ */
+
+#include <net/fib_notifier.h>
+#include <net/ip_fib.h>
+#include <net/ip6_fib.h>
+#include <net/fib_rules.h>
+#include <net/netns/generic.h>
+
+#include "netdevsim.h"
+
+struct nsim_fib_entry {
+	u64 max;
+	u64 num;
+};
+
+struct nsim_per_fib_data {
+	struct nsim_fib_entry fib;
+	struct nsim_fib_entry rules;
+};
+
+struct nsim_fib_data {
+	struct nsim_per_fib_data ipv4;
+	struct nsim_per_fib_data ipv6;
+};
+
+static unsigned int nsim_fib_net_id;
+
+u64 nsim_fib_get_val(struct net *net, enum nsim_resource_id res_id, bool max)
+{
+	struct nsim_fib_data *fib_data = net_generic(net, nsim_fib_net_id);
+	struct nsim_fib_entry *entry;
+
+	switch (res_id) {
+	case NSIM_RESOURCE_IPV4_FIB:
+		entry = &fib_data->ipv4.fib;
+		break;
+	case NSIM_RESOURCE_IPV4_FIB_RULES:
+		entry = &fib_data->ipv4.rules;
+		break;
+	case NSIM_RESOURCE_IPV6_FIB:
+		entry = &fib_data->ipv6.fib;
+		break;
+	case NSIM_RESOURCE_IPV6_FIB_RULES:
+		entry = &fib_data->ipv6.rules;
+		break;
+	default:
+		return 0;
+	}
+
+	return max ? entry->max : entry->num;
+}
+
+int nsim_fib_set_max(struct net *net, enum nsim_resource_id res_id, u64 val)
+{
+	struct nsim_fib_data *fib_data = net_generic(net, nsim_fib_net_id);
+	struct nsim_fib_entry *entry;
+	int err = 0;
+
+	switch (res_id) {
+	case NSIM_RESOURCE_IPV4_FIB:
+		entry = &fib_data->ipv4.fib;
+		break;
+	case NSIM_RESOURCE_IPV4_FIB_RULES:
+		entry = &fib_data->ipv4.rules;
+		break;
+	case NSIM_RESOURCE_IPV6_FIB:
+		entry = &fib_data->ipv6.fib;
+		break;
+	case NSIM_RESOURCE_IPV6_FIB_RULES:
+		entry = &fib_data->ipv6.rules;
+		break;
+	default:
+		return 0;
+	}
+
+	/* not allowing a new max to be less than curren occupancy
+	 * --> no means of evicting entries
+	 */
+	if (val < entry->num)
+		err = -EINVAL;
+	else
+		entry->max = val;
+
+	return err;
+}
+
+static int nsim_fib_rule_account(struct nsim_fib_entry *entry, bool add,
+				 struct netlink_ext_ack *extack)
+{
+	int err = 0;
+
+	if (add) {
+		if (entry->num < entry->max) {
+			entry->num++;
+		} else {
+			err = -ENOSPC;
+			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported fib rule entries");
+		}
+	} else {
+		entry->num--;
+	}
+
+	return err;
+}
+
+static int nsim_fib_rule_event(struct fib_notifier_info *info, bool add)
+{
+	struct nsim_fib_data *data = net_generic(info->net, nsim_fib_net_id);
+	struct netlink_ext_ack *extack = info->extack;
+	int err = 0;
+
+	switch (info->family) {
+	case AF_INET:
+		err = nsim_fib_rule_account(&data->ipv4.rules, add, extack);
+		break;
+	case AF_INET6:
+		err = nsim_fib_rule_account(&data->ipv6.rules, add, extack);
+		break;
+	}
+
+	return err;
+}
+
+static int nsim_fib_account(struct nsim_fib_entry *entry, bool add,
+			    struct netlink_ext_ack *extack)
+{
+	int err = 0;
+
+	if (add) {
+		if (entry->num < entry->max) {
+			entry->num++;
+		} else {
+			err = -ENOSPC;
+			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported fib entries");
+		}
+	} else {
+		entry->num--;
+	}
+
+	return err;
+}
+
+static int nsim_fib_event(struct fib_notifier_info *info, bool add)
+{
+	struct nsim_fib_data *data = net_generic(info->net, nsim_fib_net_id);
+	struct netlink_ext_ack *extack = info->extack;
+	int err = 0;
+
+	switch (info->family) {
+	case AF_INET:
+		err = nsim_fib_account(&data->ipv4.fib, add, extack);
+		break;
+	case AF_INET6:
+		err = nsim_fib_account(&data->ipv6.fib, add, extack);
+		break;
+	}
+
+	return err;
+}
+
+static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event,
+			     void *ptr)
+{
+	struct fib_notifier_info *info = ptr;
+	int err = 0;
+
+	switch (event) {
+	case FIB_EVENT_RULE_ADD: /* fall through */
+	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);
+}
+
+/* inconsistent dump, trying again */
+static void nsim_fib_dump_inconsistent(struct notifier_block *nb)
+{
+	struct nsim_fib_data *data;
+	struct net *net;
+
+	rcu_read_lock();
+	for_each_net_rcu(net) {
+		data = net_generic(net, nsim_fib_net_id);
+
+		data->ipv4.fib.num = 0ULL;
+		data->ipv4.rules.num = 0ULL;
+
+		data->ipv6.fib.num = 0ULL;
+		data->ipv6.rules.num = 0ULL;
+	}
+	rcu_read_unlock();
+}
+
+static struct notifier_block nsim_fib_nb = {
+	.notifier_call = nsim_fib_event_nb,
+};
+
+/* Initialize per network namespace state */
+static int __net_init nsim_fib_netns_init(struct net *net)
+{
+	struct nsim_fib_data *data = net_generic(net, nsim_fib_net_id);
+
+	data->ipv4.fib.max = (u64)-1;
+	data->ipv4.rules.max = (u64)-1;
+
+	data->ipv6.fib.max = (u64)-1;
+	data->ipv6.rules.max = (u64)-1;
+
+	return 0;
+}
+
+static struct pernet_operations nsim_fib_net_ops __net_initdata = {
+	.init = nsim_fib_netns_init,
+	.id   = &nsim_fib_net_id,
+	.size = sizeof(struct nsim_fib_data),
+};
+
+void nsim_fib_exit(void)
+{
+	unregister_pernet_subsys(&nsim_fib_net_ops);
+	unregister_fib_notifier(&nsim_fib_nb);
+}
+
+int nsim_fib_init(void)
+{
+	int err;
+
+	err = register_pernet_subsys(&nsim_fib_net_ops);
+	if (err < 0) {
+		pr_err("Failed to register pernet subsystem\n");
+		goto err_out;
+	}
+
+	err = register_fib_notifier(&nsim_fib_nb, nsim_fib_dump_inconsistent);
+	if (err < 0) {
+		pr_err("Failed to register fib notifier\n");
+		goto err_out;
+	}
+
+err_out:
+	return err;
+}
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 3fd567928f3d..8b30ab3ea2c2 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -167,6 +167,8 @@ static int nsim_init(struct net_device *dev)
 
 	SET_NETDEV_DEV(dev, &ns->dev);
 
+	nsim_devlink_setup(ns);
+
 	return 0;
 
 err_bpf_uninit:
@@ -180,6 +182,7 @@ static void nsim_uninit(struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
 
+	nsim_devlink_teardown(ns);
 	debugfs_remove_recursive(ns->ddir);
 	nsim_bpf_uninit(ns);
 }
@@ -478,12 +481,18 @@ static int __init nsim_module_init(void)
 	if (err)
 		goto err_debugfs_destroy;
 
-	err = rtnl_link_register(&nsim_link_ops);
+	err = nsim_devlink_init();
 	if (err)
 		goto err_unreg_bus;
 
+	err = rtnl_link_register(&nsim_link_ops);
+	if (err)
+		goto err_dl_fini;
+
 	return 0;
 
+err_dl_fini:
+	nsim_devlink_exit();
 err_unreg_bus:
 	bus_unregister(&nsim_bus);
 err_debugfs_destroy:
@@ -494,6 +503,7 @@ static int __init nsim_module_init(void)
 static void __exit nsim_module_exit(void)
 {
 	rtnl_link_unregister(&nsim_link_ops);
+	nsim_devlink_exit();
 	bus_unregister(&nsim_bus);
 	debugfs_remove_recursive(nsim_ddir);
 }
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index ea081c10efb8..afb8cf90c0fd 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -64,6 +64,9 @@ struct netdevsim {
 
 	bool bpf_map_accept;
 	struct list_head bpf_bound_maps;
+#if IS_ENABLED(CONFIG_NET_DEVLINK)
+	struct devlink *devlink;
+#endif
 };
 
 extern struct dentry *nsim_ddir;
@@ -103,6 +106,46 @@ nsim_bpf_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 }
 #endif
 
+#if IS_ENABLED(CONFIG_NET_DEVLINK)
+enum nsim_resource_id {
+	NSIM_RESOURCE_NONE,   /* DEVLINK_RESOURCE_ID_PARENT_TOP */
+	NSIM_RESOURCE_IPV4,
+	NSIM_RESOURCE_IPV4_FIB,
+	NSIM_RESOURCE_IPV4_FIB_RULES,
+	NSIM_RESOURCE_IPV6,
+	NSIM_RESOURCE_IPV6_FIB,
+	NSIM_RESOURCE_IPV6_FIB_RULES,
+};
+
+void nsim_devlink_setup(struct netdevsim *ns);
+void nsim_devlink_teardown(struct netdevsim *ns);
+
+int nsim_devlink_init(void);
+void nsim_devlink_exit(void);
+
+int nsim_fib_init(void);
+void nsim_fib_exit(void);
+u64 nsim_fib_get_val(struct net *net, enum nsim_resource_id res_id, bool max);
+int nsim_fib_set_max(struct net *net, enum nsim_resource_id res_id, u64 val);
+#else
+static inline void nsim_devlink_setup(struct netdevsim *ns)
+{
+}
+
+static inline void nsim_devlink_teardown(struct netdevsim *ns)
+{
+}
+
+static inline int nsim_devlink_init(void)
+{
+	return 0;
+}
+
+static inline void nsim_devlink_exit(void)
+{
+}
+#endif
+
 static inline struct netdevsim *to_nsim(struct device *ptr)
 {
 	return container_of(ptr, struct netdevsim, dev);
-- 
2.11.0

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

[Jakub Kicinski]

On Tue, 27 Mar 2018 18:22:00 -0700, David Ahern wrote:
> +void nsim_devlink_setup(struct netdevsim *ns)
> +{
> +	struct net *net = nsim_to_net(ns);
> +	bool *reg_devlink = net_generic(net, nsim_devlink_id);
> +	struct devlink *devlink;
> +	int err = -ENOMEM;
> +
> +	/* only one device per namespace controls devlink */
> +	if (!*reg_devlink) {
> +		ns->devlink = NULL;
> +		return;
> +	}
> +
> +	devlink = devlink_alloc(&nsim_devlink_ops, 0);
> +	if (!devlink)
> +		return;
> +
> +	err = devlink_register(devlink, &ns->dev);
> +	if (err)
> +		goto err_devlink_free;
> +
> +	err = devlink_resources_register(devlink);
> +	if (err)
> +		goto err_dl_unregister;
> +
> +	ns->devlink = devlink;
> +
> +	*reg_devlink = false;
> +
> +	return;
> +
> +err_dl_unregister:
> +	devlink_unregister(devlink);
> +err_devlink_free:
> +	devlink_free(devlink);
> +}

nit: DaveM expressed preference to not have silent failures in a
     discussion about DebugFS, not sure it applies here, but why not
     handle errors?

Re: [PATCH net-next 1/6] net: Fix fib notifer to return errno

[Ido Schimmel]

On Tue, Mar 27, 2018 at 06:21:55PM -0700, David Ahern wrote:
> Notifier handlers use notifier_from_errno to convert any potential error
> to an encoded format. As a consequence the other side, call_fib_notifier{s}
> in this case, needs to use notifier_to_errno to return the error from
> the handler back to its caller.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

Reviewed-by: Ido Schimmel <idosch@mellanox.com>

Re: [PATCH net-next 2/6] net: Move call_fib_rule_notifiers up in fib_nl_newrule

[Ido Schimmel]

On Tue, Mar 27, 2018 at 06:21:56PM -0700, David Ahern wrote:
> Move call_fib_rule_notifiers up in fib_nl_newrule to the point right
> before the rule is inserted into the list. At this point there are no
> more failure paths within the core rule code, so if the notifier
> does not fail then the rule will be inserted into the list.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

Reviewed-by: Ido Schimmel <idosch@mellanox.com>

Re: [PATCH net-next 2/6] net: Move call_fib_rule_notifiers up in fib_nl_newrule

[Ido Schimmel]

On Tue, Mar 27, 2018 at 06:21:56PM -0700, David Ahern wrote:
> Move call_fib_rule_notifiers up in fib_nl_newrule to the point right
> before the rule is inserted into the list. At this point there are no
> more failure paths within the core rule code, so if the notifier
> does not fail then the rule will be inserted into the list.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

BTW, this means we can now return an error from mlxsw instead of only
emitting an extack warning when an unsupported FIB rule is configured.
Will add that.

Re: [PATCH net-next 3/6] net/ipv4: Move call_fib_entry_notifiers up for new routes

[Ido Schimmel]

On Tue, Mar 27, 2018 at 06:21:57PM -0700, David Ahern wrote:
> Move call to call_fib_entry_notifiers for new IPv4 routes to right
> before the call to fib_insert_alias. At this point the only remaining
> failure path is memory allocations in fib_insert_node. Handle that
> very unlikely failure with a call to call_fib_entry_notifiers to
> tell drivers about it.
> 
> At this point notifier handlers can decide the fate of the new route
> with a clean path to delete the potential new entry if the notifier
> returns non-0.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

Reviewed-by: Ido Schimmel <idosch@mellanox.com>

Re: [PATCH net-next 4/6] net/ipv4: Allow notifier to fail route replace

[Ido Schimmel]

On Tue, Mar 27, 2018 at 06:21:58PM -0700, David Ahern wrote:
> Add checking to call to call_fib_entry_notifiers for IPv4 route replace.
> Allows a notifier handler to fail the replace.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

Reviewed-by: Ido Schimmel <idosch@mellanox.com>

Re: [PATCH net-next 5/6] net/ipv6: Move call_fib6_entry_notifiers up for route adds

[Ido Schimmel]

On Tue, Mar 27, 2018 at 06:21:59PM -0700, David Ahern wrote:
> Move call to call_fib6_entry_notifiers for new IPv6 routes to right
> before the insertion into the FIB. At this point notifier handlers can
> decide the fate of the new route with a clean path to delete the
> potential new entry if the notifier returns non-0.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

Reviewed-by: Ido Schimmel <idosch@mellanox.com>

Re: [PATCH net-next 0/6] net: Allow FIB notifiers to fail add and replace

[Ido Schimmel]

On Tue, Mar 27, 2018 at 06:21:54PM -0700, David Ahern wrote:
> I wanted to revisit how resource overload is handled for hardware offload
> of FIB entries and rules. At the moment, the in-kernel fib notifier can
> tell a driver about a route or rule add, replace, and delete, but the
> notifier can not affect the action. Specifically, in the case of mlxsw
> if a route or rule add is going to overflow the ASIC resources the only
> recourse is to abort hardware offload. Aborting offload is akin to taking
> down the switch as the path from data plane to the control plane simply
> can not support the traffic bandwidth of the front panel ports. Further,
> the current state of FIB notifiers is inconsistent with other resources
> where a driver can affect a user request - e.g., enslavement of a port
> into a bridge or a VRF.
> 
> As a result of the work done over the past 3+ years, I believe we are
> at a point where we can bring consistency to the stack and offloads,
> and reliably allow the FIB notifiers to fail a request, pushing an error
> along with a suitable error message back to the user. Rather than
> aborting offload when the switch is out of resources, userspace is simply
> prevented from adding more routes and has a clear indication of why.

Nice work, David. Ran various tests and didn't see any regressions.

I know you already know this, but for the record, we plan to add
accounting to KVD hash resources which will eventually allow us to
return errors when resources are exceeded.

Re: [PATCH net-next 0/6] net: Allow FIB notifiers to fail add and replace

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Tue, 27 Mar 2018 18:21:54 -0700

> I wanted to revisit how resource overload is handled for hardware offload
> of FIB entries and rules. At the moment, the in-kernel fib notifier can
> tell a driver about a route or rule add, replace, and delete, but the
> notifier can not affect the action. Specifically, in the case of mlxsw
> if a route or rule add is going to overflow the ASIC resources the only
> recourse is to abort hardware offload. Aborting offload is akin to taking
> down the switch as the path from data plane to the control plane simply
> can not support the traffic bandwidth of the front panel ports. Further,
> the current state of FIB notifiers is inconsistent with other resources
> where a driver can affect a user request - e.g., enslavement of a port
> into a bridge or a VRF.
> 
> As a result of the work done over the past 3+ years, I believe we are
> at a point where we can bring consistency to the stack and offloads,
> and reliably allow the FIB notifiers to fail a request, pushing an error
> along with a suitable error message back to the user. Rather than
> aborting offload when the switch is out of resources, userspace is simply
> prevented from adding more routes and has a clear indication of why.
> 
> This set does not resolve the corner case where rules or routes not
> supported by the device are installed prior to the driver getting loaded
> and registering for FIB notifications. In that case, hardware offload has
> not been established and it can refuse to offload anything, sending
> errors back to userspace via extack. Since conceptually the driver owns
> the netdevices associated with its asic, this corner case mainly applies
> to unsupported rules and any races during the bringup phase.
> 
> Patch 1 fixes call_fib_notifiers to extract the errno from the encoded
> response from handlers.
> 
> Patches 2-5 allow the call to call_fib_notifiers to fail the add or
> replace of a route or rule.
> 
> Patch 6 adds a simple resource controller to netdevsim to illustrate
> how a FIB resource controller can limit the number of route entries.

Series applied, thanks David.

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

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 27 Mar 2018 18:34:50 -0700

> On Tue, 27 Mar 2018 18:22:00 -0700, David Ahern wrote:
>> +void nsim_devlink_setup(struct netdevsim *ns)
>> +{
 ...
> nit: DaveM expressed preference to not have silent failures in a
>      discussion about DebugFS, not sure it applies here, but why not
>      handle errors?

Yes it is a concern.

David please address this as a follow-up.

Thanks.

Re: [PATCH net-next 0/6] net: Allow FIB notifiers to fail add and replace

[David Ahern]

On 3/29/18 2:29 AM, Ido Schimmel wrote:
> Nice work, David. Ran various tests and didn't see any regressions.
> 
> I know you already know this, but for the record, we plan to add
> accounting to KVD hash resources which will eventually allow us to
> return errors when resources are exceeded.

Thanks for running the tests, and looking forward to the mlxsw updates
for resource management.

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

[David Ahern]

On 3/29/18 12:11 PM, David Miller wrote:
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Tue, 27 Mar 2018 18:34:50 -0700
> 
>> On Tue, 27 Mar 2018 18:22:00 -0700, David Ahern wrote:
>>> +void nsim_devlink_setup(struct netdevsim *ns)
>>> +{
>  ...
>> nit: DaveM expressed preference to not have silent failures in a
>>      discussion about DebugFS, not sure it applies here, but why not
>>      handle errors?
> 
> Yes it is a concern.
> 
> David please address this as a follow-up.

Will do.

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

[Jiri Pirko]

Wed, Mar 28, 2018 at 03:22:00AM CEST, dsa@cumulusnetworks.com wrote:
>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.

I don't understand why you add 1:1 fixed relationship between
netnamespace and devlink instance. That is highly misleading and reader
might think that those 2 are somehow related. They are not. You can have
multiple devlink instances for many ports in a single namespace.

Could you please clarify?

Also, to see the relationship between individual netdevsim netdevices
and the parent devlink instance, we should use devlink_port
instances, like this: 

      devlink1              devlink2
       |    |                |    |
 dl_port1_1 dlport1_2   dlport2_1 dlport2_2
       |    |                |    |
     eth0  eth1             eth2 eth3

Note that "devlink instance" reprensents one ASIC.
The address of the devlink instance is the bus address of the ASIC.
Here, you use address of some/first netdevsim netdev instance.

The way it is implemented in netdevsim by this patch is wrong on
so many levels :(

Could you please fix this? I'm more than happy to help you with this,
please say so. Thanks!


[...]

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


this goto is pointless. Just return.

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

[David Ahern]

On 4/5/18 11:27 AM, Jiri Pirko wrote:
> Wed, Mar 28, 2018 at 03:22:00AM CEST, dsa@cumulusnetworks.com wrote:
>> 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.
> 
> I don't understand why you add 1:1 fixed relationship between
> netnamespace and devlink instance. That is highly misleading and reader
> might think that those 2 are somehow related. They are not. You can have
> multiple devlink instances for many ports in a single namespace.

The netdevsim devlink instance is an example of limiting the number of
FIB entries and FIB rules for a namespace. It is currently limited to
the init_net based on past discussion.

It does not make sense to have multiple resource controllers for the
same network namespace, hence the limit of only registering with devlink
on the first device create.

> 
> Could you please clarify?
> 
> Also, to see the relationship between individual netdevsim netdevices
> and the parent devlink instance, we should use devlink_port
> instances, like this: 
> 
>       devlink1              devlink2
>        |    |                |    |
>  dl_port1_1 dlport1_2   dlport2_1 dlport2_2
>        |    |                |    |
>      eth0  eth1             eth2 eth3
> 
> Note that "devlink instance" reprensents one ASIC.
> The address of the devlink instance is the bus address of the ASIC.
> Here, you use address of some/first netdevsim netdev instance.

The ASIC here is the kernel tables in a namespace. It does not make
sense to have 2 devlink instances for a single namespace.

> 
> The way it is implemented in netdevsim by this patch is wrong on
> so many levels :(
> 
> Could you please fix this? I'm more than happy to help you with this,
> please say so. Thanks!

What is there to fix?

Not creating a netdevsim device per netdevsim netdevice? That is
completely unrelated to the devlink change.

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

[David Ahern]

On 4/5/18 2:10 PM, David Ahern wrote:
> 
> The ASIC here is the kernel tables in a namespace. It does not make
> sense to have 2 devlink instances for a single namespace.

I put this example controller in netdevsim per a suggestion from Ido.
The netdevsim seemed like a good idea given that modules intention --
testing network facilities. Perhaps I should have done this as a
completely standalone module ...

The intention is to treat the kernel's tables *per namespace* as a
standalone entity that can be managed very similar to ASIC resources.
Given that I can add a resource controller module
(drivers/net/kern_res_mgr.c?) that creates a 'struct device' per network
namespace with a devlink instance. In this case the device would very
much be tied to the namespace 1:1.

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

[Jiri Pirko]

Thu, Apr 05, 2018 at 10:10:29PM CEST, dsa@cumulusnetworks.com wrote:
>On 4/5/18 11:27 AM, Jiri Pirko wrote:
>> Wed, Mar 28, 2018 at 03:22:00AM CEST, dsa@cumulusnetworks.com wrote:
>>> 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.
>> 
>> I don't understand why you add 1:1 fixed relationship between
>> netnamespace and devlink instance. That is highly misleading and reader
>> might think that those 2 are somehow related. They are not. You can have
>> multiple devlink instances for many ports in a single namespace.
>
>The netdevsim devlink instance is an example of limiting the number of
>FIB entries and FIB rules for a namespace. It is currently limited to
>the init_net based on past discussion.
>
>It does not make sense to have multiple resource controllers for the
>same network namespace, hence the limit of only registering with devlink
>on the first device create.

Devlink instance represents an ASIC. 1:1. There is no relation with
network namespaces and should not be. I have no clue why you think so.

The model looks as I described it down below in the picture.


>
>> 
>> Could you please clarify?
>> 
>> Also, to see the relationship between individual netdevsim netdevices
>> and the parent devlink instance, we should use devlink_port
>> instances, like this: 
>> 
>>       devlink1              devlink2
>>        |    |                |    |
>>  dl_port1_1 dlport1_2   dlport2_1 dlport2_2
>>        |    |                |    |
>>      eth0  eth1             eth2 eth3
>> 
>> Note that "devlink instance" reprensents one ASIC.
>> The address of the devlink instance is the bus address of the ASIC.
>> Here, you use address of some/first netdevsim netdev instance.
>
>The ASIC here is the kernel tables in a namespace. It does not make
>sense to have 2 devlink instances for a single namespace.

Again. No clue why you build relationship with namespace.


>
>> 
>> The way it is implemented in netdevsim by this patch is wrong on
>> so many levels :(
>> 
>> Could you please fix this? I'm more than happy to help you with this,
>> please say so. Thanks!
>
>What is there to fix?
>
>Not creating a netdevsim device per netdevsim netdevice? That is
>completely unrelated to the devlink change.

To fit the model. Multiple devlink instances, each representing one
"virtual" ASIC, devlink_port instances, 1 for each netdevsim port.
Netdevsim port should simulate real devices. No real device should have
1:1 relation with network namespace. That is just simply wrong.

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

[Jiri Pirko]

Thu, Apr 05, 2018 at 11:06:41PM CEST, dsa@cumulusnetworks.com wrote:
>On 4/5/18 2:10 PM, David Ahern wrote:
>> 
>> The ASIC here is the kernel tables in a namespace. It does not make
>> sense to have 2 devlink instances for a single namespace.
>
>I put this example controller in netdevsim per a suggestion from Ido.
>The netdevsim seemed like a good idea given that modules intention --
>testing network facilities. Perhaps I should have done this as a
>completely standalone module ...
>
>The intention is to treat the kernel's tables *per namespace* as a
>standalone entity that can be managed very similar to ASIC resources.

So you say you want to treat a namespace as an ASIC? That sounds very
odd to me :/


>Given that I can add a resource controller module
>(drivers/net/kern_res_mgr.c?) that creates a 'struct device' per network
>namespace with a devlink instance. In this case the device would very
>much be tied to the namespace 1:1.

That sounds more reasonable and accurate, yet still odd. You would not
have any netdevices there? Any ports?

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

[David Ahern]

On 4/5/18 11:52 PM, Jiri Pirko wrote:
> Thu, Apr 05, 2018 at 11:06:41PM CEST, dsa@cumulusnetworks.com wrote:
>> On 4/5/18 2:10 PM, David Ahern wrote:
>>>
>>> The ASIC here is the kernel tables in a namespace. It does not make
>>> sense to have 2 devlink instances for a single namespace.
>>
>> I put this example controller in netdevsim per a suggestion from Ido.
>> The netdevsim seemed like a good idea given that modules intention --
>> testing network facilities. Perhaps I should have done this as a
>> completely standalone module ...
>>
>> The intention is to treat the kernel's tables *per namespace* as a
>> standalone entity that can be managed very similar to ASIC resources.
> 
> So you say you want to treat a namespace as an ASIC? That sounds very
> odd to me :/

Why? The kernel has forwarding tables, acl's, etc just like the ASIC,
and each namespace is a separate set of tables.

If you think about it, userspace "programs" the kernel just like mlxsw
and userspace SDKs "program" an asic.


>> Given that I can add a resource controller module
>> (drivers/net/kern_res_mgr.c?) that creates a 'struct device' per network
>> namespace with a devlink instance. In this case the device would very
>> much be tied to the namespace 1:1.
> 
> That sounds more reasonable and accurate, yet still odd. You would not
> have any netdevices there? Any ports?
> 

Sure, what ever ports are assigned to or created in the namespace.

Nothing about the devlink API says it has to be a real h/w device.
Nothing about the devlink API says it can only be used for real h/w that
has ports represented by netdevices that the devlink instance some how
has "control" over.

As the netdevsim demo shows, I can build an L3 resource controller for
the kernel tables using just the devlink API and the in-kernel notifiers.

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

[Jiri Pirko]

Fri, Apr 06, 2018 at 11:22:29PM CEST, dsa@cumulusnetworks.com wrote:
>On 4/5/18 11:52 PM, Jiri Pirko wrote:
>> Thu, Apr 05, 2018 at 11:06:41PM CEST, dsa@cumulusnetworks.com wrote:
>>> On 4/5/18 2:10 PM, David Ahern wrote:
>>>>
>>>> The ASIC here is the kernel tables in a namespace. It does not make
>>>> sense to have 2 devlink instances for a single namespace.
>>>
>>> I put this example controller in netdevsim per a suggestion from Ido.
>>> The netdevsim seemed like a good idea given that modules intention --
>>> testing network facilities. Perhaps I should have done this as a
>>> completely standalone module ...
>>>
>>> The intention is to treat the kernel's tables *per namespace* as a
>>> standalone entity that can be managed very similar to ASIC resources.
>> 
>> So you say you want to treat a namespace as an ASIC? That sounds very
>> odd to me :/
>
>Why? The kernel has forwarding tables, acl's, etc just like the ASIC,
>and each namespace is a separate set of tables.

I don't get it. What's the point? For HW, the reason is it has limited
resources and those resources are not mapped 1:1 with kernel object.
However, for kernel, that is meaningless.


>
>If you think about it, userspace "programs" the kernel just like mlxsw
>and userspace SDKs "program" an asic.

I don't give a **** about sdks. I have no clue why you mention that here.


>
>
>>> Given that I can add a resource controller module
>>> (drivers/net/kern_res_mgr.c?) that creates a 'struct device' per network
>>> namespace with a devlink instance. In this case the device would very
>>> much be tied to the namespace 1:1.
>> 
>> That sounds more reasonable and accurate, yet still odd. You would not
>> have any netdevices there? Any ports?
>> 
>
>Sure, what ever ports are assigned to or created in the namespace.
>
>Nothing about the devlink API says it has to be a real h/w device.

Sure, it could represent something made-up, like netdevsim. However I
see a big misfit when you want to represent a namespace.


>Nothing about the devlink API says it can only be used for real h/w that
>has ports represented by netdevices that the devlink instance some how
>has "control" over.
>
>As the netdevsim demo shows, I can build an L3 resource controller for
>the kernel tables using just the devlink API and the in-kernel notifiers.