I hadn't even thought of that, and since you're seeing the ingress dscp, it must be working that way. Being a programmer I was thinking of it like a stack with call/return vs goto, but that must be wrong. Sorry for the noise!
It was for the same reason... to only classify on new connections. It's more efficient to do that than to classify every packet. The ct mark bit is essentially tracking what conntrack is already tracking for you, which is why the bit is redundant.
Ah, I understand now. It was only until @dave14305 brought in the ct mark & 128 == 0 check that this was actually leveraged.
@fta does this look correct to you:
Sure does!
Any ideas about how to have the cake-qos-simple service script apply firewall rules itself rather than separate .nft file?
See here:
Also, I want to separate out the configuration parameters from the service script. Having those in the service script makes maintaining the service script a pain.
So the assumption that makes caching the initial egress DSCP unconditionally a good idea is that DSCP-using applications do not change the used DSCP later. Has anybody actually confirmed whether that assumption holds?
Good and interesting point. Certainly when I use local policies in Windows those are fixed, but individual applications could still presumably vary DSCPs on existing connections. Although presumably that'd be rather unusual?
For folks here using Comcast that reported the CS1 issue - please note that by roughly end of day today, all our config files will be updated so that you should see CS0. If you see this after end of day please shoot me an email (jason_livingood@comcast.com). Most CMTSes are updated - just a few remain to update today. 
I restructured cake-qos-simple:
Now:
Looking great after rebooting the modem!
@moeller0 / @dave14305, I traced an issue with excessive throttling on the LG when streaming certain videos.
It's my ECN scrubbing, and specifically that in the download direction:
if [[ -n "${overwrite_ecn_val_dl}" ]]
then
printf "\nSetting up filter to overwrite ecn bits to decimal value: '${overwrite_ecn_val_dl}' on download.\n"
tc filter add dev "${dl_if}" parent 1: protocol ip matchall action pedit ex munge ip dsfield set "${overwrite_ecn_val_dl}" retain 0x3
tc filter add dev "${dl_if}" parent 1: protocol ipv6 matchall action pedit ex munge ip6 traffic_class set "${overwrite_ecn_val_dl}" retain 0x3
fi
root@OpenWrt-1:~/cake-qos-simple# tc -s filter show dev ifb-wan
filter parent 1: protocol ipv6 pref 49151 matchall chain 0
filter parent 1: protocol ipv6 pref 49151 matchall chain 0 handle 0x1
not_in_hw (rule hit 1)
action order 1: pedit action pass keys 1
index 4 ref 1 bind 1 installed 309 sec used 205 sec firstused 205 sec
key #0 at ipv6+0: val 00000000 mask ffcfffff
Action statistics:
Sent 174 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
filter parent 1: protocol ip pref 49152 matchall chain 0
filter parent 1: protocol ip pref 49152 matchall chain 0 handle 0x1
not_in_hw (rule hit 30960)
action order 1: pedit action pass keys 1
index 3 ref 1 bind 1 installed 309 sec used 1 sec firstused 309 sec
key #0 at ipv4+0: val 00000000 mask fffcffff
Action statistics:
Sent 38490263 bytes 31113 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
Any clue what I've got wrong?
Technically, I guess you should only clear ECT(0) and ECT(1), if you remove CE mark you should actually drop that packet instead otherwise you remove a 'slow down' signal from somewhere along that path. If you disable your ECN cleaning and capture some traffic of the LG, which ECN bits do you actually see its traffic use? Because if it does not use ECN in the first place this might mean something in your filter might have side effects or operate differently to what we expect...
On 'wan' I see:
18:33:48.124956 IP (tos 0x2,ECT(0), ttl 59, id 22365, offset 0, flags [DF], proto TCP (6), length 789)
server-54-182-171-100.lhr61.r.cloudfront.net.80 > LGwebOSTV.lan.58590: Flags [P.], cksum 0xa847 (incorrect -> 0x4c14), seq 383244:383981, ack 1, win 1702, options [nop,nop,TS val 4254665186 ecr 2188886], length 737: HTTP
...
18:42:31.553362 IP (tos 0x2a,ECT(0), ttl 58, id 14616, offset 0, flags [DF], proto TCP (6), length 1360)
server-54-182-171-105.lhr61.r.cloudfront.net.80 > LGwebOSTV.lan.60942: Flags [.], cksum 0x276d (correct), seq 15697:17005, ack 376, win 2104, options [nop,nop,TS val 4255188707 ecr 2241238], length 1308: HTTP
These ECN bits are certainly getting scrubbed at 'ifb-wan' given my scrubbing code, but this seems to be wreaking havoc in terms of permitting transfer of this video stream.
Any clue why?
Is the answer that scrubbing ECN bits on download breaks the TCP handshake or something? But it's OK on upload?
I'm now no longer sure about this whole idea about scrubbing ECN bits before cake sees the packets. Can we say conclusively whether this will or will not break stuff?
Depending on your answer I'm thinking I should perhaps remove the experimental scrub ECN functionality from cake-qos-simple.
No, removing ECT(0) and ECT(1) should be detected and result in both endpoints not using ECN. Removing CE is a different kettle of fish though, a rfc3168 AQM will CE-mark ECT(0|1) packets instead of dropping them, so later removing that CE will result in that AQMs signal not making it to the end-points which can result in in the flow keeping sending too much (until the AQM switches from CE marking to dropping) resulting in too much queued data and likely massive dropping events. However for this to happen in your case there would need to be a rfc3168 AQM on the LG path that starts CE marking before your own cake instances, which IMHO is quite unlikely (given how rare rfc3168 AQM are on the internet according too a number of reports trying to quantify their number). But my point stands, CE marked packets should not be ECN cleared...
As long as we only fiddle with the ECN bits and all checksums are correctly updated after the changes this should not break stuff (modulo the CE issue mentioned above).
Also for testing it might be helpful to completely disable the DSCP propagation functionality, this should not be involved, but this is easy to test, and since ECN and DS bitfields are close by, it is possible that some bug is twiddling the wrong bits somehow?
But does this include doing it in every packet. Not just handshake but every packet? On upload and download.
So should the overwrite be conditional on ECN bits not equaling '3'? So 0, 1 or 2 can be overwritten to user-defined value, but 3 should be left alone?
Ah, perhaps that's the issue!
https://man7.org/linux/man-pages/man8/tc-csum.8.html
But then which headers do I include?
TARGET Specify which headers to update: IPv4 header (ip4h), ICMP header (icmp), IGMP header (igmp), TCP header (tcp), UDP header (udp), UDPLite header (udplite) or SCTP header (sctp).
And does this include IPv6 support?
If the handshake negotiates no ECN then no packet should contain ECT(0) or ECT(1), let alone CE, having these different from Not-ECT (all two bits set to zero) is a bug and should not happen.
Technically one should not change Not-ECT (00) to anything else (except for testing where arbitrary values are helpful), so essentially you would need to test 3>ECN>0 or force the rewrite value to be zero...
Well, I bet it is ip4h as that is where the ECN bitfield lives (note the IPv6 header does not contain a header checksum):
Sweet. Let me retest with the csum ip4h added and see if this fixes the weird issue.
I just note this is subtle, both UDP and TCP take part of the IP header (as pseudoheader) into account for their own checksums, so depending on which fields you edit you also need to recalculate the UDP/TCP checksums. However as far as I see (so not very far) neither the DS nor the ECN fields are included in these pseudoheaders so this complication hopefully is not needed for your usage.
Hmm... seems just adding 'csum ip4h' didn't work.
Time for packet captures, I think that wireshark shows wrong checksums...