Date:   Thu, 4 Jan 2018 11:24:03 +0200
From:   Arkadi Sharshevsky <arkadis@...lanox.com>
To:     David Ahern <dsa@...ulusnetworks.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 01/04/2018 04:28 AM, David Ahern wrote:
> 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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Farkadis%2Flibmnl.git&data=02%7C01%7Carkadis%40mellanox.com%7C9a369b54cdec48a5e1d208d5531adbdb%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636506297202356822&sdata=6KOkBz5PHqu6z8nlexSdggZj42LE4geiYVFA%2BgcgaPE%3D&reserved=0
>>
>> 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).
>

Double word is 64 bit, dont understand why this is confusing.

> $ 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.

You are clearly misunderstanding what is occupancy of the resource
and what kvd linear is good for. As I mentioned in the last response
kvd linear is mapped to adjacency table. So in case its 0 no nexthop
routes could be configured, this information is provided by the
dpipe<-->resource.

Occupancy means how much of the resource is used right now, why is
this wrong? and how its related to the size 0 exactly?

> 
> 
> 
> 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.
> 

I would like more adding the granularity size to the extack string
instead of adding this to UAPI. The user will try to update once
and will get the required granularity in the error message.

> 
> 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."
> 

No problem, sounds better.

> 
> 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.
>

Strongly against it.

Powered by blists - more mailing lists