From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next 6/6] netdevsim: Add simple FIB resource controller via devlink Date: Thu, 5 Apr 2018 19:27:18 +0200 Message-ID: <20180405172718.GA9125@nanopsycho> References: <20180328012200.15175-1-dsa@cumulusnetworks.com> <20180328012200.15175-7-dsa@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, davem@davemloft.net, roopa@cumulusnetworks.com, shm@cumulusnetworks.com, jiri@mellanox.com, idosch@mellanox.com, jakub.kicinski@netronome.com, andy.roulin@gmail.com To: David Ahern Return-path: Received: from mail-wr0-f176.google.com ([209.85.128.176]:38786 "EHLO mail-wr0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751412AbeDER1U (ORCPT ); Thu, 5 Apr 2018 13:27:20 -0400 Received: by mail-wr0-f176.google.com with SMTP id m13so29712969wrj.5 for ; Thu, 05 Apr 2018 10:27:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20180328012200.15175-7-dsa@cumulusnetworks.com> Sender: netdev-owner@vger.kernel.org List-ID: 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, >+ ¶ms, NULL); >+ if (err) { >+ pr_err("Failed to register IPv4 top resource\n"); >+ goto out; this goto is pointless. Just return.