From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [patch net-next v2 00/10] Add support for resource abstraction Date: Wed, 3 Jan 2018 19:28:31 -0700 Message-ID: <48d2d512-6879-cbce-16a4-3413f6505c3d@cumulusnetworks.com> References: <20171226112359.5313-1-jiri@resnulli.us> <977652df-a0ed-d1a5-f299-1dc433ebd337@mellanox.com> <96389ae0-e038-8a26-84ea-0cf1b9fa0a05@cumulusnetworks.com> <0f861e90-63d3-2666-ef2d-0fc91beae957@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, mlxsw@mellanox.com, andrew@lunn.ch, vivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com, michael.chan@broadcom.com, ganeshgr@chelsio.com, saeedm@mellanox.com, matanb@mellanox.com, leonro@mellanox.com, idosch@mellanox.com, jakub.kicinski@netronome.com, ast@kernel.org, daniel@iogearbox.net, simon.horman@netronome.com, pieter.jansenvanvuuren@netronome.com, john.hurley@netronome.com, alexander.h.duyck@intel.com, linville@tuxdriver.com, gospo@broadcom.com, steven.lin1@broadcom.com, yuvalm@mellanox.com, ogerlitz@mellanox.com To: Arkadi Sharshevsky , Jiri Pirko , netdev@vger.kernel.org, roopa@cumulusnetworks.com Return-path: Received: from mail-pl0-f54.google.com ([209.85.160.54]:45398 "EHLO mail-pl0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751945AbeADC2f (ORCPT ); Wed, 3 Jan 2018 21:28:35 -0500 Received: by mail-pl0-f54.google.com with SMTP id o2so228469plk.12 for ; Wed, 03 Jan 2018 18:28:35 -0800 (PST) In-Reply-To: <0f861e90-63d3-2666-ef2d-0fc91beae957@mellanox.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 1/3/18 11:05 AM, Arkadi Sharshevsky wrote: > > > On 01/02/2018 08:05 PM, David Ahern wrote: >> On 1/1/18 7:58 AM, Arkadi Sharshevsky wrote: >>> >>> Just to summarize the current fixes required: >>> >>> 1. ERIF dpipe table size is reporting wrong size. More precisely the >>> ERIF table does not take rifs, so it should not be linked to the rif >>> bank resource (is not part of this patchset, future extension). >>> 2. Extended ACK user-space bug. >>> 3. ABI documentation- Not sure we agreed upon it, Jiri? >>> >>> If I missed something please respond. Nothing of the fixes mentioned >>> above is relevant for this patchset actually. >>> >> >> Can you fix the userspace command and then we come back to what else is >> needed? Right now, it is hard to tell what is a user space bug and what >> is a kernel space bug. >> >> For example: >> $ devlink resource set pci/0000:03:00.0 path /kvd/linear size 10000 >> $ devlink resource show pci/0000:03:00.0 >> pci/0000:03:00.0: >> name kvd size 245760 size_valid true >> resources: >> name linear size 98304 occ 0 >> name hash_double size 60416 >> name hash_single size 87040 >> >> The set command did not fail, yet there is no size_new arg in the output >> like there is for this change: >> >> $ devlink resource set pci/0000:03:00.0 path /kvd/linear size 0 >> $ devlink resource show pci/0000:03:00.0 >> pci/0000:03:00.0: >> name kvd size 245760 size_valid true >> resources: >> name linear size 98304 size_new 0 occ 0 >> name hash_double size 60416 >> name hash_single size 87040 >> > > As I stated this is a user-space bug which I fixed, and updated my repo > so please pull. Devlink uses mnl,and currently mnl does not support > extended ack. I added support for this in my local ver of libmnl: > > https://github.com/arkadis/libmnl.git > > On branch master, so you can check it out. Besides this bugs, which were > userspace, can please specify what are the pending problems from your > point of view? Thanks! > Again, my comments all stem from user experience. Can you explain what "double_word" means for a unit? I would expect a units to be kB or count (or items or entries). $ devlink resource show pci/0000:03:00.0 pci/0000:03:00.0: name kvd size 245760 unit double_word size_valid true resources: name linear size 98304 occ 0 unit double_word name hash_double size 60416 unit double_word name hash_single size 87040 unit double_word While that is confusing here from the userspace command it goes hand in hand with patch 2 and: +enum devlink_resource_unit { + DEVLINK_RESOURCE_UNIT_DOUBLE_WORD, +}; Also, it seems like the occ of 0 is wrong since we know from past responses that if I set linear to 0 all of networking breaks. How does a user learn the granularity of a resource: $ devlink resource set pci/0000:03:00.0 path /kvd/hash_double size 50000 Error: mlxsw_spectrum: resource set with wrong granularity. Try again with 51000 and then 52000 and ... Why not export the granularity read-only? I don't see it in the proposed UAPI. And then on the reload: $ devlink reload pci/0000:03:00.0 Error: devlink: resources size validation failed. Since the reload is not doing any resource sizing that error message is confusing. Maybe something like "Sum of the resource components exceeds total size." Finally, I still contend a 1-line description of each of the resources goes a long way to improving the user friendliness of this set.