netdev - Re: [PATCH net-next 0/5] devlink rate police limiter

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]

Date:   Thu, 30 Jun 2022 17:27:08 +0200
From:   Dima Chumak <dchumak@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Jiri Pirko <jiri@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 0/5] devlink rate police limiter

On 6/20/22 10:04 PM, Jakub Kicinski wrote:
> 
> On Mon, 20 Jun 2022 18:26:42 +0300 Dima Chumak wrote:
>> Currently, kernel provides a way to limit tx rate of a VF via devlink
>> rate function of a port. The underlying mechanism is a shaper applied to
>> all traffic passing through the target VF or a group of VFs. By its
>> essence, a shaper naturally works with outbound traffic, and in
>> practice, it's rarely seen to be implemented for inbound traffic.
>> Nevertheless, there is a user request to have a mechanism for limiting
>> inbound traffic as well. It is usually done by using some form of
>> traffic policing, dropping excess packets over the configured limit that
>> set by a user. Thus, introducing another limiting mechanism to the port
>> function can help close this gap.
>>
>> This series introduces devlink attrs, along with their ops, to manage
>> rate policing of a single port as well as a port group. It is based on
>> the existing notion of leaf and node rate objects, and extends their
>> attributes to support both RX and TX limiting, for a number of packets
>> per second and/or a number of bytes per second. Additionally, there is a
>> second set of parameters for specifying the size of buffering performed,
>> called "burst", that controls the allowed level of spikes in traffic
>> before it starts getting dropped.
>>
>> A new sub-type of a devlink_rate object is introduced, called
>> "limit_type". It can be either "shaping", the default, or "police".
>> A single leaf or a node object can be switched from one limit type to
>> another, but it cannot do both types of rate limiting simultaneously.
>> A node and a leaf object that have parent-child relationship must have
>> the same limit type. In other words, it's only possible to group rate
>> objects of the same limit type as their group's limit_type.
> 
> TC already has the police action. Your previous patches were accepted
> because there was no exact match for shaping / admission. Now you're
> "extending" that API to duplicate existing TC APIs. Infuriating.

I'm sorry for not being able to reply promptly.

I've re-read more carefully the cover letter of the original 'devlink:
rate objects API' series by Dmytro Linkin, off of which I based my
patches, though my understanding still might be incomplete/incorrect
here.

It seems that TC, being ingress only, doesn't cover the full spectrum of
rate-limiting that's possible to achieve with devlink. TC works only
with representors and doesn't allow to configure "the other side of the
wire", where devlink port function seems to be a better match as it
connects directly to a VF.

Also, for the existing devlink-rate mechanism of VF grouping, it would be
challenging to achieve similar functionality with TC flows, as groups don't
have a net device instance where flows can be attached.

I want to apologize in case my proposed changes have come across as
being bluntly ignoring some of the pre-established agreements and
understandings of TC / devlink responsibility separation, it wasn't
intentional.