From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f194.google.com ([209.85.216.194]:41150 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751409AbeCXDry (ORCPT ); Fri, 23 Mar 2018 23:47:54 -0400 Received: by mail-qt0-f194.google.com with SMTP id d18so2224023qtl.8 for ; Fri, 23 Mar 2018 20:47:54 -0700 (PDT) Date: Fri, 23 Mar 2018 20:47:49 -0700 From: Jakub Kicinski To: David Ahern Cc: netdev@vger.kernel.org, davem@davemloft.net, roopa@cumulusnetworks.com, shm@cumulusnetworks.com, jiri@mellanox.com, idosch@mellanox.com, David Ahern Subject: Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink Message-ID: <20180323204749.52d30129@cakuba.netronome.com> In-Reply-To: <20180322225757.10377-8-dsa@cumulusnetworks.com> References: <20180322225757.10377-1-dsa@cumulusnetworks.com> <20180322225757.10377-8-dsa@cumulusnetworks.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote: > From: 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. 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, > + ¶ms, 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, > + ¶ms, &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, > + ¶ms, &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, > + ¶ms, 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, > + ¶ms, &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, > + ¶ms, &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); > +}