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

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 05/12/15 13:03, Alexei Starovoitov wrote:
> On Tue, May 12, 2015 at 05:16:46PM +0200, Florian Westphal wrote:
>>>
>>> A qdisc might have a stab (cf qdisc_calculate_pkt_len() )
>>
>> Thanks for pointing this out Eric.
>
> Indeed. Thanks Eric!
>
>> I was under impression that stab was only useful for egress but in
>> fact tc did support ingress stab too.
>
> That was my impression as well. Though it was allowed to add
> qdisc_size_table to ingress, it's useless. Nothing takes advantage
> of recomputed qdisc_pkt_len. It can only mess with stats, which
> seems to be already broken:
> - egress qdiscs do qdisc_bstats_update() only at dequeue, whereas
>    ingress double counts dropped packets
> - qdisc_bstats_update() does:
>    bstats->bytes += qdisc_pkt_len(skb);
>    bstats->packets += skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
>    but nothing on ingress does qdisc_pkt_len_init().
>    So when we see gso packet on ingress the stats are very wrong.
>
> I think we should fix ingress stats as a whole.
> Option 1:
>   do qdisc_pkt_len_init() + optional call into size_table
>   to update stats only after classifers returned TC_ACT_OK.
>   Cons: extra overhead per packet only to update ingress qdisc stats.
> Option 2:
>   use byte and packet counts from underlying netdev, when
>   reporting stats via ingress qdisc.
>   Pros: No arithmetic in the fast path.
>
> I think option 2 is much preferred, since it's faster and
> equally accurate.
>
> Jamal, what's your take?
>

I dont think we need the stab on the ingress but we do need to account
for gso. So option #1 with qdisc_pkt_len_init() alone is the only thing
needed. i.e Florian's change becomes:

-	qdisc_bstats_update_cpu(cl->q, skb);
+	qdisc_pkt_len_init(skb)
  	skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
+	qdisc_bstats_update_cpu(cl->q, skb);

Alexei, why do you say this option will have overhead?

cheers,
jamal