Re: [Cake] [PATCH for v5.9] sch_cake: Replace HTTP links with HTTPS ones

[David Miller]

Please don't do this "for v5.9" stuff.

There is no precendence for this and it looks like -stable patch
series to just about anyone.

There are well defined, established, ways to write Subject lines for
proper patch submissions.

Please do not invent your own way of doing this.  It is very
frustrating for me personally, especially with the amount of
patches I have to process and review, to watch how you wildly
change your patch submission formatting over and over again.

Just do what other developers are doing and you'll be fine.  Don't
invent your own way.

I'm ignoring all of your "for v5.9" formatted patches, sorry.

Thanks.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net v2] vlan: consolidate VLAN parsing code and limit max parsing depth

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Tue,  7 Jul 2020 13:03:25 +0200

> Toshiaki pointed out that we now have two very similar functions to extract
> the L3 protocol number in the presence of VLAN tags. And Daniel pointed out
> that the unbounded parsing loop makes it possible for maliciously crafted
> packets to loop through potentially hundreds of tags.
> 
> Fix both of these issues by consolidating the two parsing functions and
> limiting the VLAN tag parsing to a max depth of 8 tags. As part of this,
> switch over __vlan_get_protocol() to use skb_header_pointer() instead of
> pskb_may_pull(), to avoid the possible side effects of the latter and keep
> the skb pointer 'const' through all the parsing functions.
> 
> v2:
> - Use limit of 8 tags instead of 32 (matching XMIT_RECURSION_LIMIT)
> 
> Reported-by: Toshiaki Makita 
> Reported-by: Daniel Borkmann 
> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in 
> the presence of VLANs")
> Signed-off-by: Toke Høiland-Jørgensen 

Applied, thank you.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net v3] sched: consistently handle layer3 header accesses in the presence of VLANs

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Fri,  3 Jul 2020 22:26:43 +0200

> There are a couple of places in net/sched/ that check skb->protocol and act
> on the value there. However, in the presence of VLAN tags, the value stored
> in skb->protocol can be inconsistent based on whether VLAN acceleration is
> enabled. The commit quoted in the Fixes tag below fixed the users of
> skb->protocol to use a helper that will always see the VLAN ethertype.
> 
> However, most of the callers don't actually handle the VLAN ethertype, but
> expect to find the IP header type in the protocol field. This means that
> things like changing the ECN field, or parsing diffserv values, stops
> working if there's a VLAN tag, or if there are multiple nested VLAN
> tags (QinQ).
> 
> To fix this, change the helper to take an argument that indicates whether
> the caller wants to skip the VLAN tags or not. When skipping VLAN tags, we
> make sure to skip all of them, so behaviour is consistent even in QinQ
> mode.
> 
> To make the helper usable from the ECN code, move it to if_vlan.h instead
> of pkt_sched.h.
> 
> v3:
> - Remove empty lines
> - Move vlan variable definitions inside loop in skb_protocol()
> - Also use skb_protocol() helper in IP{,6}_ECN_decapsulate() and
>   bpf_skb_ecn_set_ce()
> 
> v2:
> - Use eth_type_vlan() helper in skb_protocol()
> - Also fix code that reads skb->protocol directly
> - Change a couple of 'if/else if' statements to switch constructs to avoid
>   calling the helper twice
> 
> Reported-by: Ilya Ponetayev 
> Fixes: d8b9605d2697 ("net: sched: fix skb->protocol use in case of 
> accelerated vlan path")
> Signed-off-by: Toke Høiland-Jørgensen 

Looks good, applied and queued up for -stable.

Thanks!
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH RESEND net-next] sch_cake: add RFC 8622 LE PHB support to CAKE diffserv handling

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 25 Jun 2020 22:18:00 +0200

> From: Kevin Darbyshire-Bryant 
> 
> Change tin mapping on diffserv3, 4 & 8 for LE PHB support, in essence
> making LE a member of the Bulk tin.
> 
> Bulk has the least priority and minimum of 1/16th total bandwidth in the
> face of higher priority traffic.
> 
> NB: Diffserv 3 & 4 swap tin 0 & 1 priorities from the default order as
> found in diffserv8, in case anyone is wondering why it looks a bit odd.
> 
> Signed-off-by: Kevin Darbyshire-Bryant 
> [ reword commit message slightly ]
> Signed-off-by: Toke Høiland-Jørgensen 

Applied, thanks!
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net 0/3] sched: A couple of fixes for sch_cake

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 25 Jun 2020 22:12:06 +0200

> This series contains a couple of fixes for diffserv handling in sch_cake that
> provide a nice speedup (with a somewhat pedantic nit fix tacked on to the 
> end).
> 
> Not quite sure about whether this should go to stable; it does provide a nice
> speedup, but it's not strictly a fix in the "correctness" sense. I lean 
> towards
> including this in stable as well, since our most important consumer of that
> (OpenWrt) is likely to backport the series anyway.

Series applied and queued up for -stable, thanks.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next 1/5] sch_cake: fix IP protocol handling in the presence of VLAN tags

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 25 Jun 2020 21:53:53 +0200

> I think it depends a little on the use case; some callers actually care
> about the VLAN tags themselves and handle that specially (e.g.,
> act_csum). Whereas others (e.g., sch_dsmark) probably will have the same
> issue. I guess I can trying going through them all and figuring out if
> there's a more generic solution.

That makes sense.

> I'll split out the diffserv parsing fixes and send those for your net
> tree straight away, then circle back to this one...

Great, thank you.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next 0/5] sched: A series of fixes and optimisations for sch_cake

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 25 Jun 2020 13:55:02 +0200

> The first three patches in the series are candidates for inclusion into 
> stable.

Stable candidates, ie. fixes, should target 'net' not 'net-next'.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next 1/5] sch_cake: fix IP protocol handling in the presence of VLAN tags

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 25 Jun 2020 13:55:03 +0200

> From: Ilya Ponetayev 
> 
> CAKE was using the return value of tc_skb_protocol() and expecting it to be
> the IP protocol type. This can fail in the presence of QinQ VLAN tags,
> making CAKE unable to handle ECN marking and diffserv parsing in this case.
> Fix this by implementing our own version of tc_skb_protocol(), which will
> use skb->protocol directly, but also parse and skip over any VLAN tags and
> return the inner protocol number instead.
> 
> Also fix CE marking by implementing a version of INET_ECN_set_ce() that
> uses the same parsing routine.
> 
> Fixes: ea82511518f4 ("sch_cake: Add NAT awareness to packet classifier")
> Fixes: b2100cc56fca ("sch_cake: Use tc_skb_protocol() helper for getting 
> packet protocol")
> Fixes: 046f6fd5daef ("sched: Add Common Applications Kept Enhanced (cake) 
> qdisc")
> Signed-off-by: Ilya Ponetayev 
> [ squash original two patches, rewrite commit message ]
> Signed-off-by: Toke Høiland-Jørgensen 

First, this is a bug fix and should probably be steered to 'net'.

Also, other users of tc_skb_protocol() are almost certainly hitting a
similar problem aren't they?  Maybe fix this generically.

___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net] sch_cake: Take advantage of skb->hash where appropriate

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Fri, 29 May 2020 14:43:44 +0200

> While the other fq-based qdiscs take advantage of skb->hash and doesn't
> recompute it if it is already set, sch_cake does not.
> 
> This was a deliberate choice because sch_cake hashes various parts of the
> packet header to support its advanced flow isolation modes. However,
> foregoing the use of skb->hash entirely loses a few important benefits:
> 
> - When skb->hash is set by hardware, a few CPU cycles can be saved by not
>   hashing again in software.
> 
> - Tunnel encapsulations will generally preserve the value of skb->hash from
>   before the encapsulation, which allows flow-based qdiscs to distinguish
>   between flows even though the outer packet header no longer has flow
>   information.
> 
> It turns out that we can preserve these desirable properties in many cases,
> while still supporting the advanced flow isolation properties of sch_cake.
> This patch does so by reusing the skb->hash value as the flow_hash part of
> the hashing procedure in cake_hash() only in the following conditions:
> 
> - If the skb->hash is marked as covering the flow headers (skb->l4_hash is
>   set)
> 
> AND
> 
> - NAT header rewriting is either disabled, or did not change any values
>   used for hashing. The latter is important to match local-origin packets
>   such as those of a tunnel endpoint.
> 
> The immediate motivation for fixing this was the recent patch to WireGuard
> to preserve the skb->hash on encapsulation. As such, this is also what I
> tested against; with this patch, added latency under load for competing
> flows drops from ~8 ms to sub-1ms on an RRUL test over a WireGuard tunnel
> going through a virtual link shaped to 1Gbps using sch_cake. This matches
> the results we saw with a similar setup using sch_fq_codel when testing the
> WireGuard patch.
> 
> Fixes: 046f6fd5daef ("sched: Add Common Applications Kept Enhanced (cake) 
> qdisc")
> Signed-off-by: Toke Høiland-Jørgensen 

Applied to net-next, thanks.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH] net/sch_generic.h: use sizeof_member() and get rid of unused variable

[David Miller]

From: Antonio Quartulli 
Date: Wed, 20 May 2020 10:39:33 +0200

> I don't think it's BUILD_BUG_ON()'s fault, because qcb->data is passed
> to sizeof() first.
> 
> My best guess is that gcc is somewhat optimizing the sizeof(gcb->data)
> and thus leaving the gcb variable unused.

If you remove the argument from the function but leave the BUILD_BUG_ON()
calls the same, the compilation will fail.

Any such optimization is therefore unreasonable.

The variable is used otherwise compilation would not fail when you
remove it right?
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH] net/sch_generic.h: use sizeof_member() and get rid of unused variable

[David Miller]

From: Antonio Quartulli 
Date: Tue, 19 May 2020 11:13:33 +0200

> Compiling with -Wunused triggers the following warning:
> 
> ./include/net/sch_generic.h: In function ‘qdisc_cb_private_validate’:
> ./include/net/sch_generic.h:464:23: warning: unused variable ‘qcb’ 
> [-Wunused-variable]
>   464 |  struct qdisc_skb_cb *qcb;
>   |   ^~~
> 
> as the qcb variable is only used to compute the sizeof one of its members.

It's referenced in the code, therefore it is not "unused".

If in some configuration BUILD_BUG_ON() does not reference it's arguments,
that's the bug that needs to be fixed.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net 0/2] sched: A few small fixes for sch_cake

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 04 Apr 2019 15:01:33 +0200

> Hi Dave
> 
> Kevin noticed a few issues with the way CAKE reads the skb protocol and the IP
> diffserv fields. This series fixes those two issues, and should probably go to
> in 4.19 as well. However, the previous refactoring patch means they don't 
> apply
> as-is; I can send a follow-up directly to stable if that's OK with you?

Series applied, and queued up for -stable.

Indeed, posting a backport for v4.19 would be great.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net] sch_cake: Interpret fwmark parameter as a bitmask

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 14 Mar 2019 23:08:22 +0100

> We initially interpreted the fwmark parameter as a flag that simply turned
> on the feature, using the whole skb->mark field as the index into the CAKE
> tin_order array. However, it is quite common for different applications to
> use different parts of the mask field for their own purposes, each using a
> different mask.
> 
> Support this use of subsets of the mark by interpreting the TCA_CAKE_FWMARK
> parameter as a bitmask to apply to the fwmark field when reading it. The
> result will be right-shifted by the number of unset lower bits of the mask
> before looking up the tin.
> 
> In the original commit message we also failed to credit Felix Resch with
> originally suggesting the fwmark feature back in 2017; so the Suggested-By
> in this commit covers the whole fwmark feature.
> 
> Fixes: 0b5c7efdfc6e ("sch_cake: Permit use of connmarks as tin classifiers")
> Suggested-by: Felix Resch 
> Signed-off-by: Toke Høiland-Jørgensen 

You are lucky you decided to do this before a released kernel had this UAPI
available.

Applied.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v1 0/3] sched: Patches from out-of-tree version of sch_cake

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Fri, 01 Mar 2019 16:04:05 +0100

> This series includes a couple of patches with updates from the out-of-tree
> version of sch_cake. The first one is a fix to the fairness scheduling when
> dual-mode fairness is enabled. The second patch is an additional feature flag
> that allows using fwmark as a tin selector, as a convenience for people who 
> want
> to customise tin selection. The third patch is just a cleanup to the tin
> selection logic.

Series applied, thanks Toke.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH 0/4] sched: Fix qdisc interactions exposed by using sch_cake as a leaf qdisc

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Mon,  7 Jan 2019 20:47:29 +0100

> This series fixes a couple of issues exposed by running sch_cake as a
> leaf qdisc in an HFSC tree, which were discovered and reported by Pete
> Heist. The interaction between CAKE's GSO splitting and the parent
> qdisc's notion of its own queue length could cause queue stalls. While
> investigating the report, I also noticed that several qdiscs would
> dereference the skb pointer after dequeue, which is potentially
> problematic since the GSO splitting code also frees the original skb.
> 
> See the individual patches in the series for details.

Toke, can you please resubmit this without patch #3.

If you want to push for patch #3 still, it is much easier to submit
it separately.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH] MAINTAINERS: Add entry for CAKE qdisc

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Fri, 16 Nov 2018 12:13:59 -0800

> We would like the existing community to be kept in the loop for any new
> developments on CAKE; and I certainly plan to keep maintaining it. Reflect
> this in MAINTAINERS.
> 
> Signed-off-by: Toke Høiland-Jørgensen 

Applied, thanks Toke.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net v2] gso_segment: Reset skb->mac_len after modifying network header

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Thu, 13 Sep 2018 16:43:07 +0200

> When splitting a GSO segment that consists of encapsulated packets, the
> skb->mac_len of the segments can end up being set wrong, causing packet
> drops in particular when using act_mirred and ifb interfaces in
> combination with a qdisc that splits GSO packets.
> 
> This happens because at the time skb_segment() is called, network_header
> will point to the inner header, throwing off the calculation in
> skb_reset_mac_len(). The network_header is subsequently adjust by the
> outer IP gso_segment handlers, but they don't set the mac_len.
> 
> Fix this by adding skb_reset_mac_len() calls to both the IPv4 and IPv6
> gso_segment handlers, after they modify the network_header.
> 
> Many thanks to Eric Dumazet for his help in identifying the cause of
> the bug.
> 
> Acked-by: Dave Taht 
> Reviewed-by: Eric Dumazet 
> Signed-off-by: Toke Høiland-Jørgensen 
> ---
> v2:
>   - Properly credit Eric for his help
>   - Add review and ack tags

Applied and queued up for -stable, thanks.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next] sch_cake: Make gso-splitting configurable from userspace

[David Miller]

From: Dave Taht 
Date: Thu, 26 Jul 2018 19:45:09 -0700

> I expect the first part of this patch to generate no controversy,
> as being able to enable configure gso-splitting on or off in all
> use cases of cake is a goodness.
> 
> But: I expect the single line re-enabling cake's fielded default of
> always splitting gro and gso packets, in shaped or unshaped mode, back
> into packets, to reduce my email systems' hard disk inbox to a barren,
> burnt cylinder, even if it is made easy to override thusly:
> 
> tc qdisc replace dev whatever root cake no-split-gso
> 
> While I agree that gro/gso is needed at 10gigE+ speeds, I feel
> offering an option to disable splitting to those users trying to run
> cake at those speeds is better than the alternative of forcing users
> running at 1Gbit, 100mbit, 10mbit and below, with and without pause
> frames, shaped or unshaped, to remember to split-gso.
> 
> While I have assembled tons of data in use cases ranging from nearly 0
> to a gbit, the first, and most compelling argument I can make is
> made in the commit that follows, where allowing GSO/GRO superpackets
> triples the size of the underlying BQL when running at a gbit.

I've applied this, we can revert it if there are strong objections.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next] sch_cake: Fix tin order when set through skb->priority

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Mon, 16 Jul 2018 16:45:09 +0200

> In diffserv mode, CAKE stores tins in a different order internally than
> the logical order exposed to userspace. The order remapping was missing
> in the handling of 'tc filter' priority mappings through skb->priority,
> resulting in bulk and best effort mappings being reversed relative to
> how they are displayed.
> 
> Fix this by adding the missing mapping when reading skb->priority.
> 
> Fixes: 83f8fd69af4f ("sch_cake: Add DiffServ handling")
> Signed-off-by: Toke Høiland-Jørgensen 

Applied, thanks.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v18 0/8] sched: Add Common Applications Kept Enhanced (cake) qdisc

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Fri, 01 Jun 2018 19:42:12 +0200

> Toke Høiland-Jørgensen  writes:
> 
>> This patch series adds the CAKE qdisc, and has been split up to ease
>> review.
>>
>> I have attempted to split out each configurable feature into its own patch.
>> The first commit adds the base shaper and packet scheduler, while
>> subsequent commits add the optional features. The full userspace API and
>> most data structures are included in this commit, but options not
>> understood in the base version will be ignored.
> 
> Hmm, there seems to be a lockup issue being triggered when running CAKE
> at high speeds (>40 Gbps in my tests). Please drop this series for now,
> we'll return for 4.19 with a revised version.

Ok.

Thanks for continuing to push this work along.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v15 4/7] sch_cake: Add NAT awareness to packet classifier

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Wed, 23 May 2018 22:38:30 +0200

> How would this work?

On egress the core networking flow dissector records what you need
somewhere in SKB or wherever.  You later retrieve it at egress time
after NAT has occurred.

> It's about making sure the per-host fairness works when NATing, so
> we can distribute bandwidth between the hosts on the local LAN
> regardless of how many flows they open.

Ok, understood.

> But it's not unreasonable to expect people who do NAT in eBPF to
> also set skb->tc_classid if they want pre-nat host fairness, is it?

And core networking can do it as well.

Please remove this conntrack dependency, I don't think it is necessary
and it is very short sighted.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v15 4/7] sch_cake: Add NAT awareness to packet classifier

[David Miller]

From: Jonathan Morton 
Date: Wed, 23 May 2018 23:33:04 +0300

> Now I'm *really* confused.
> 
> Are you saying that the user has to set up their own conntrack
> mechanism using extra userspace commands?  Because complicating the
> setup process that way runs directly counter to Cake's design
> philosophy.

I mean not anything filtering or firewall related.

We have a full flow dissector in the networking core, which often runs
on every RX packet anyways.  Record what we need and use it on egress
after NAT has occurred.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v15 4/7] sch_cake: Add NAT awareness to packet classifier

[David Miller]

From: Jonathan Morton 
Date: Wed, 23 May 2018 22:31:53 +0300

> Remember that it takes two different qdiscs to implement ingress and
> egress on the same physical interface, and there's no obvious
> logical link between them - especially since the ingress one has to
> be attached to an ifb, not to the actual interface, because there's
> no native support for ingress qdiscs.

Who said anything about using an ingress qdisc to record/remember
this information?
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v15 4/7] sch_cake: Add NAT awareness to packet classifier

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Tue, 22 May 2018 15:57:38 +0200

> When CAKE is deployed on a gateway that also performs NAT (which is a
> common deployment mode), the host fairness mechanism cannot distinguish
> internal hosts from each other, and so fails to work correctly.
> 
> To fix this, we add an optional NAT awareness mode, which will query the
> kernel conntrack mechanism to obtain the pre-NAT addresses for each packet
> and use that in the flow and host hashing.
> 
> When the shaper is enabled and the host is already performing NAT, the cost
> of this lookup is negligible. However, in unlimited mode with no NAT being
> performed, there is a significant CPU cost at higher bandwidths. For this
> reason, the feature is turned off by default.
> 
> Cc: netfilter-de...@vger.kernel.org
> Signed-off-by: Toke Høiland-Jørgensen 

This is really pushing the limits of what a packet scheduler can
require for correct operation.  And this creates an incredibly
ugly dependency.

I'd much rather you do something NAT method agnostic, like save
or compute the necessary information on ingress and then later
use it on egress.

Because what you have here will completely break when someone does NAT
using eBPF, act_nat, or similar.

There is even skb->rxhash, be creative :-)
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v11 1/7] sched: Add Common Applications Kept Enhanced (cake) qdisc

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Tue, 15 May 2018 17:12:44 +0200

> +typedef u64 cobalt_time_t;
> +typedef s64 cobalt_tdiff_t;
 ...
> +static cobalt_time_t cobalt_get_time(void)
> +{
> + return ktime_get_ns();
> +}
> +
> +static u32 cobalt_time_to_us(cobalt_time_t val)
> +{
> + do_div(val, NSEC_PER_USEC);
> + return (u32)val;
> +}

If fundamentally you are working with ktime_t values, please use that type
and the associated helpers.

This is a valid argument that using custom typedefs provide documentation
and an aid to understanding, but I think it doesn't serve that purpose
very well here.

So please just use ktime_t throughout instead of this cobalt_time_t
and cobalt_tdiff_t.  And then use helpers like ktime_to_us() which
properly optimize for 64-bit vs. 32-bit hosts.

Thank you.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v9 1/7] sched: Add Common Applications Kept Enhanced (cake) qdisc

[David Miller]

From: Toke Høiland-Jørgensen <t...@toke.dk>
Date: Mon, 14 May 2018 11:08:28 +0200

> David Miller <da...@davemloft.net> writes:
> 
>> From: Toke Høiland-Jørgensen <t...@toke.dk>
>> Date: Tue, 08 May 2018 16:34:19 +0200
>>
>>> +struct cake_flow {
>>> +   /* this stuff is all needed per-flow at dequeue time */
>>> +   struct sk_buff*head;
>>> +   struct sk_buff*tail;
>>
>> Please do not invent your own SKB list handling mechanism.
> 
> We didn't invent it, we inherited it from fq_codel. I was actually about
> to fix that, but then I noticed it was still around in fq_codel, and so
> let it be. I can certainly fix it anyway, but, erm, why is it acceptable
> in fq_codel but not in cake? struct sk_buff_head is not that new, is it?

I guess one argument has to do with the amount of memory consumed by this
per-flow or per-queue information, right?  Because the skb queue head has
a qlen and a spinlock regardless of whether they are used or not.

Furthermore, if you use the __skb_insert() et al. helpers, even though it
won't use the lock it will adjust the qlen counter.  And that's useless
work since you have no use for the qlen value.

Taken together, it seems that what you and fq_codel are doing is not
such a bad idea after all.  So please leave it alone.

On-and-off again, I've looked into converting skbs to using list_head
but it's a non-trivial set of work.  All over the tree the different
layers use the next/prev pointers in different ways.  Some use it for
a doubly linked list.  Some use it for a singly linked list.  Some
encode state in the prev pointer.  You name it, it's out there.

I'll try to get back to that task because obviously it'll be useful to
have code like cake and fq_codel use common helpers instead of custom
stuff.

Thanks.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] [PATCH net-next v9 1/7] sched: Add Common Applications Kept Enhanced (cake) qdisc

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Tue, 08 May 2018 16:34:19 +0200

> +struct cake_flow {
> + /* this stuff is all needed per-flow at dequeue time */
> + struct sk_buff*head;
> + struct sk_buff*tail;

Please do not invent your own SKB list handling mechanism.

Use a standard sk_buff_head.  If you don't need the locking etc., we
have interfaces like __skb_queue_head_init(), __skb_insert(), etc.
which will accomodate that.

> +static void cake_heapify(struct cake_sched_data *q, u16 i)
> +{
> + static const u32 a = CAKE_MAX_TINS * CAKE_QUEUES;
> + u32 m = i;
> + u32 mb = cake_heap_get_backlog(q, m);

Please order local variables from longest to shortest line.

The entire submissions has this problem all over the place, please
correct it patch-series wide.

Thank you.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

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

[David Miller]

From: Eric Dumazet 
Date: Tue, 1 May 2018 12:12:45 -0700

> It seems you guys spent years/months on work on this stuff, so what
> is the big deal about presenting your work in the best possible way
> ?

+1
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

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

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Wed, 25 Apr 2018 20:34:23 +0200

> 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...

Yes it is.

Otherwise the software GSO fallback wouldn't even be possible.
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake

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

[David Miller]

From: Toke Høiland-Jørgensen 
Date: Wed, 25 Apr 2018 15:42:48 +0200

> +static void *cake_zalloc(size_t sz)
> +{
> + void *ptr = kzalloc(sz, GFP_KERNEL | __GFP_NOWARN);
> +
> + if (!ptr)
> + ptr = vzalloc(sz);
> + return ptr;
> +}

This is just kvzalloc(sz, GFP_KERNEL | __GFP_NOWARN)?
___
Cake mailing list
Cake@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/cake