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.
