Date:   Thu, 10 Nov 2022 17:54:03 +0100
From:   "Wilczynski, Michal" <michal.wilczynski@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     <netdev@...r.kernel.org>, <alexandr.lobakin@...el.com>,
        <jacob.e.keller@...el.com>, <jesse.brandeburg@...el.com>,
        <przemyslaw.kitszel@...el.com>, <anthony.l.nguyen@...el.com>,
        <ecree.xilinx@...il.com>, <jiri@...nulli.us>
Subject: Re: [PATCH net-next v10 10/10] ice: add documentation for
 devlink-rate implementation



On 11/9/2022 10:25 PM, Jakub Kicinski wrote:
> On Wed, 9 Nov 2022 19:54:52 +0100 Wilczynski, Michal wrote:
>> On 11/8/2022 11:39 PM, Jakub Kicinski wrote:
>>> On Mon,  7 Nov 2022 19:13:26 +0100 Michal Wilczynski wrote:
>>>> Add documentation to a newly added devlink-rate feature. Provide some
>>>> examples on how to use the features, which netlink attributes are
>>>> supported and descriptions of the attributes.
>>>> +Devlink Rate
>>>> +==========
>>>> +
>>>> +The ``ice`` driver implements devlink-rate API. It allows for offload of
>>>> +the Hierarchical QoS to the hardware. It enables user to group Virtual
>>>> +Functions in a tree structure and assign supported parameters: tx_share,
>>>> +tx_max, tx_priority and tx_weight to each node in a tree. So effectively
>>>> +user gains an ability to control how much bandwidth is allocated for each
>>>> +VF group. This is later enforced by the HW.
>>>> +
>>>> +It is assumed that this feature is mutually exclusive with DCB and ADQ, or
>>>> +any driver feature that would trigger changes in QoS, for example creation
>>>> +of the new traffic class.
>>> Meaning? Will the devlink API no longer reflect reality once one of
>>> the VFs enables DCB for example?
>> By DCB I mean the DCB that's implemented in the FW, and I'm not aware
>> of any flow that would enable the VF to tweak FW DCB on/off. Additionally
>> there is a commit in this patch series that should prevent any devlink-rate
>> changes if the FW DCB is enabled, and should prevent enabling FW DCB
>> enablement if any changes were made with the devlink-rate.
> Nice, but in case DCB or TC/ADQ gets enabled devlink rate will just
> show a stale hierarchy?

Yes there will be hierarchy exported during the VF creation, so if
the user enable DCB/ADQ in the meantime, it will be a stale hierarchy.
User won't be able to modify any nodes/parameters.

I will clarify this also in v11.

>
> We need to document clearly that the driver is supposed to prevent
> multiple APIs being used, and how we decide which API takes precedence.

OK, agree will do that in v11.

>
>> I don't think there is a way to detect that the SW DCB is enabled though.
>> In that case the software would try to enforce it's own settings in the SW
>> stack and the HW would try to enforce devlink-rate settings.
>>
>>>> +        consumed by the tree Node. Rate Limit is an absolute number
>>>> +        specifying a maximum amount of bytes a Node may consume during
>>>> +        the course of one second. Rate limit guarantees that a link will
>>>> +        not oversaturate the receiver on the remote end and also enforces
>>>> +        an SLA between the subscriber and network provider.
>>>> +    * - ``tx_share``
>>> Wouldn't it be more common to call this tx_min, like in the old VF API
>>> and the cgroup APIs?
>> I agree on this one, I'm not really sure why this attribute is called
>> tx_share. In it's iproute documentation tx_share is described as:
>> "specifies minimal tx rate value shared among all rate objects. If rate
>> object is a part of some rate group, then this value shared with rate
>> objects of this rate group.".
>> So tx_min is more intuitive, but I suspect that the original author
>> wanted to emphasize that this BW is shared among all the children
>> nodes.
> Ah :/ I missed you're not adding this one :S

Powered by blists - more mailing lists