Re: Flow classifier proto-dst and TOS (and proto-src)

[Eric Dumazet <eric.dumazet@gmail.com>]

Le dimanche 23 octobre 2011 à 21:03 -0400, Dan Siemon a écrit :
> On Mon, 2011-10-17 at 08:09 +0200, Eric Dumazet wrote:
> > Le samedi 15 octobre 2011 à 12:51 -0400, Dan Siemon a écrit :
> > > cls_flow.c: flow_get_proto_dst()
> > > 
> > > The proto-dst key returns the destination port for UDP, TCP and a few
> > > other protocols [see proto_ports_offset()]. For ICMP and IPIP it falls
> > > back to:
> > > 
> > > return addr_fold(skb_dst(skb)) ^ (__force u16)skb->protocol;
> > > 
> > > Since Linux maintains a dst_entry for each TOS value this causes the
> > > returned value to be affected by the TOS which is unexpected and
> > > probably broken.
> > 
> > Hi Dan
> > 
> > I think Patrick did this on purpose, because of of the lack of
> > perturbation in cls_flow.c : If all these frames were mapped to a single
> > flow, they might interfere with an other regular flow and hurt it.
> > 
> > I dont qualify existing code as buggy. Its about fallback behavior
> > anyway (I dont think its even documented)
> 
> Thanks for the review Eric.
> 
> Won't virtually all uses of proto-dst also use the dst key anyway? In
> which case this fallback does nothing except make the TOS effect the
> hash output because the dst will be the same and dst_entry would be the
> same if it wasn't for the different TOS (by far the common case). I
> don't see the value of the unintuitive behavior.
> 
> I'm not certain this is a problem but also note that including TOS will
> mean that packets within a tunnel will be reordered if 'tos inherit' is
> set on the tunnel and only the typical src,dst,proto,proto-src,proto-dst
> is used. Again, probably not expected.

Hmm, it seems cls_flow now supports "perturbation", so my prior argument
is not true anymore. I guess your patch would be fine, but I prefer
waiting Patrick review on it ;)