netdev - Re: [patch net-next v2 00/10] Add support for resource abstraction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <48d2d512-6879-cbce-16a4-3413f6505c3d@cumulusnetworks.com>
Date:   Wed, 3 Jan 2018 19:28:31 -0700
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Arkadi Sharshevsky <arkadis@...lanox.com>,
        Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        roopa@...ulusnetworks.com
Cc:     davem@...emloft.net, mlxsw@...lanox.com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        michael.chan@...adcom.com, ganeshgr@...lsio.com,
        saeedm@...lanox.com, matanb@...lanox.com, leonro@...lanox.com,
        idosch@...lanox.com, jakub.kicinski@...ronome.com, ast@...nel.org,
        daniel@...earbox.net, simon.horman@...ronome.com,
        pieter.jansenvanvuuren@...ronome.com, john.hurley@...ronome.com,
        alexander.h.duyck@...el.com, linville@...driver.com,
        gospo@...adcom.com, steven.lin1@...adcom.com, yuvalm@...lanox.com,
        ogerlitz@...lanox.com
Subject: Re: [patch net-next v2 00/10] Add support for resource abstraction

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.