Re: [PATCH net-next v10 10/10] ice: add documentation for devlink-rate implementation

[Jakub Kicinski <kuba@kernel.org>]

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?

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

> 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