On Fri, 23 Aug 2013 14:37:47 +0200 Sebastian Moeller <[email protected]> wrote:
> On Aug 23, 2013, at 13:16 , Jesper Dangaard Brouer <[email protected]> wrote: > > > On Fri, 23 Aug 2013 12:15:12 +0200 > > Sebastian Moeller <[email protected]> wrote: [...] > >>>>> Especially since the kernel already fudges > >>>>> the packet size to account for the ethernet header and then some, so > >>>>> this > >>>>> path should receive more scrutiny by virtue of having more users? > >>> > >>> As you mention, the default kernel path (not tc stab) fudges the packet > >>> size for Ethernet headers, AND I made a mistake (back in approx 2006, > >>> sorry) that the "overhead" cannot be a negative number. > >> > >> Mmh, does this also apply to stab? > > > > This seems to be two question... > > > > Yes, the Ethernet header size gets adjusted/added before the "stab" > > call. > > For reference > > See: net/core/dev.c function __dev_xmit_skb() > > Call qdisc_pkt_len_init(skb); // adjust Ethernet and account for GSO > > Call qdisc_calculate_pkt_len(skb, q); // is the stab call > > (ps calls __qdisc_calculate_pkt_len() in net/sched/sch_api.c) > > > > The qdisc_pkt_len_init() call were introduced by Eric in > > v3.9-rc1~139^2~411. > > So I look at 3.10 here: > > net/core/dev.c, qdisc_pkt_len_init > line 2628: qdisc_skb_cb(skb)->pkt_len = skb->len; > and in > line 2650: qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len; > so the adjusted size does not seem to end in skb->len > > > and then in > net/sched/sch_api.c, __qdisc_calculate_pkt_len > line440: pkt_len = skb->len + stab->szopts.overhead; > > So to my eyes this looks like stab is not honoring the changes made in > qdisc_pkt_len_init, no? At least I fail to see where > skb->len is assigned qdisc_skb_cb(skb)->pkt_len > But I happily admit that I am truly a novice in these matters and easily > intimidated by C code. You are absolutely correct, and I were wrong. Guess I didn't read __qdisc_calculate_pkt_len() carefully enough, sorry. When stab is enabled, it will override the skb pkt_len. > > Thus, in kernels >= 3.9, you would need to change/reduce your tc > > "overhead" parameter with -14 bytes (iif you accounted encapsulated > > Ethernet header before) > > That is what I thought before, but my kernel spelunking made > me reconsider and switch to not subtract the 14 bytes since as I > understand it the kernel actively does not do it if stab is used. You are correct, and I was wrong. Good to have more eyeballs on the code :-) -- Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn: http://www.linkedin.com/in/brouer _______________________________________________ Cerowrt-devel mailing list [email protected] https://lists.bufferbloat.net/listinfo/cerowrt-devel
