netdev - Re: [Cake] [PATCH net-next v3] Add Common Applications Kept Enhanced (cake) qdisc

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]

Message-ID: <874ljz5c87.fsf@toke.dk>
Date:   Wed, 25 Apr 2018 21:15:36 +0200
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        Jonathan Morton <chromatix99@...il.com>
Cc:     netdev@...r.kernel.org, cake@...ts.bufferbloat.net
Subject: Re: [Cake] [PATCH net-next v3] Add Common Applications Kept Enhanced (cake) qdisc

Eric Dumazet <eric.dumazet@...il.com> writes:

> On 04/25/2018 11:34 AM, Toke Høiland-Jørgensen wrote:
>> Eric Dumazet <eric.dumazet@...il.com> writes:
>> 
>>> On 04/25/2018 09:52 AM, Jonathan Morton wrote:
>>>>> We can see here the high cost of forcing software GSO :/
>>>>>
>>>>> Really, this should be done only :
>>>>> 1) If requested by the admin ( tc .... gso ....)
>>>>>
>>>>> 2) If packet size is above a threshold.
>>>>>  The threshold could be set by the admin, and/or based on a fraction of the bandwidth parameter.
>>>>>
>>>>> I totally understand why you prefer to segment yourself for < 100 Mbit links.
>>>>>
>>>>> But this makes no sense on 10Gbit+
>>>>
>>>> It is absolutely necessary, so far as I can see, to segment GSO
>>>> superpackets when overhead compensation is selected - as it very
>>>> often should be, even on pure Ethernet links. Without that, the
>>>> calculation of link occupancy time will be wrong. (The actual
>>>> transmission time of an Ethernet frame is rather more than just 14
>>>> bytes longer than the underlying IP packet.)
>>>
>>> Just fix the overhead compensation computation in the code.
>>>
>>> skb in a qdisc have everything you need.
>>>
>>> qdisc_pkt_len_init() has initialized qdisc_skb_cb(skb)->pkt_len with
>>> the exact bytes on the wire, and you have gso_segs to perform any
>>> adjustement you need to do.
>> 
>> The problem is that may not be the right values. For example, in many
>> CPEs there's a built-in switch that strips VLAN tags before the packet
>> actually hits the wire. So we do need to be able to get the actual
>> packet size. Is it possible to get the sizes of the individual segments
>> of a GSO packet? That way we could do the calculation for the whole
>> super-packet...
>
> All segments of  GSO packets have the same size, by definition.
>
> Only the last segment might be smaller, and again this can be inferred
> from gso_size and gso_segs

Gotcha. Until we are confident that we've implemented this in a way that
works, will this do?

@@ -88,6 +88,7 @@
 #define CAKE_SET_WAYS (8)
 #define CAKE_MAX_TINS (8)
 #define CAKE_QUEUES (1024)
+#define CAKE_SPLIT_GSO_THRESHOLD (125000000) /* 1Gbps */
@@ -1437,7 +1439,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
         * or if we need to know individual packet sizes for framing overhead.
         */
 
-       if (skb_is_gso(skb)) {
+       if (skb_is_gso(skb) && q->rate_flags & CAKE_FLAG_SPLIT_GSO) {
                struct sk_buff *segs, *nskb;
                netdev_features_t features = netif_skb_features(skb);
                /* signed slen to handle corner case
@@ -2337,6 +2339,12 @@ static int cake_change(struct Qdisc *sch, struct nlattr *opt,
        if (tb[TCA_CAKE_MEMORY])
                q->buffer_config_limit = nla_get_u32(tb[TCA_CAKE_MEMORY]);
 
+       if (q->rate_bps && (q->rate_bps <= CAKE_SPLIT_GSO_THRESHOLD ||
+                           q->rate_flags & CAKE_FLAG_OVERHEAD))
+               q->rate_flags |= CAKE_FLAG_SPLIT_GSO;
+       else
+               q->rate_flags &= ~CAKE_FLAG_SPLIT_GSO;
+
        if (q->tins) {
                sch_tree_lock(sch);
                cake_reconfigure(sch);