Re: [PATCH net-next v2] net: core: set and refine qdisc pkt len before tc_classify

On Wed, 2015-05-13 at 23:33 +0200, Florian Westphal wrote:

Eric Dumazet [off-list ref] wrote:

quoted

quoted

Commit d2788d34885d4ce5ba ("net: sched: further simplify handle_ing")
removed the call to qdisc_enqueue_root(). However, after this removal
we no longer set qdisc pkt length, which this patch fixes. We make use
of qdisc_pkt_len_init() from 1def9238d4aa ("net_sched: more precise
pkt_len computation"), which was suggested as the current approach so
far does not take into account when GRO builds up GSO packets.

Fixes: d2788d34885d ("net: sched: further simplify handle_ing")
Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 v1->v2:
  - use qdisc_pkt_len_init() as suggested, thanks!

@@ -3541,9 +3541,11 @@ static inline struct sk_buff *handle_ing(struct sk_buff *skb,
 		*pt_prev = NULL;
 	}
 
-	qdisc_bstats_update_cpu(cl->q, skb);
+	qdisc_pkt_len_init(skb);

[..]

quoted

Is qdisc_pkt_len_init() still inlined, now it has 2 callers ?

gcc 4.8.4, OPTIMIZE_FOR_SIZE=n: nope, callq.

quoted

People using policers on ingress are very often disabling GRO for
various historical reasons.

right, ingress policing is rather poor with GRO (or you need to set mtu 64000
on the tc police cmd line).

quoted

Since handle_ing() is very uncommon, I would prefer not slowing down tx
just so that GRO ~5% error in ingress is fixed.

Hmm, so whats you suggestion?  Just use

qdisc_skb_cb(skb)->pkt_len = skb->len;

like in v1 of the patch set?
resurrect qdisc_enqueue_root?
set always_inline (i'd prefer not, its not very small)?

Any other idea?

One possibility would be to set it in GRO engine.

But then we would have to set qdisc_skb_cb(skb)->pkt_len = skb->len for
non GRO packets.

Also note the DODGY thing might be not needed for ingress,
and I am not sure we can currently deliver UDP aggregated packets.

Hmm....