Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov <ast@plumgrid.com>]

On 4/9/15 3:49 AM, Jamal Hadi Salim wrote:
> those devices and only speacial case with them. Your assumptions of
> blindly pushing/pulling will break  in some cases (take a look at
> mirred).

you underestimated how much time I've spent studying mirred ;)
In particular my v2 patch fixes two bugs in it:
- it was forwarding packets with L2 present to tunnel devices if used
   with egress qdisc
- it wasn't updating skb->csum with ingress qdisc

In v3 since 'needs_l2' is an optional flag, I didn't touch mirred
and left its bugs for the future patches.

> Your changes penalize everyone else because of this assumption
> bpf makes. We have always tried to be sensitive to perfomance.

hmm. penalize everyone? it's an optional flag.
If ingress_l2 is used with cls_bpf plus some other classifier,
sure, the other classifier is paying potential cost skb_share_check
if taps are active. The chances of having more than a couple classifiers
on a single device are very slim, since they're called sequentially
and killing performance regardless.
In most of the use cases I'm after, cls_bpf will be the only classifier
attached. In some cases there may be few others, but cls_bpf will be
the first and the heaviest one. So it makes sense to optimize for it.

> code path and you are adding a bunch more unconditionally.

not true at all. Looks like you misread the v3 patch completely.
It's an optional flag.