Re: [PATCH net-next-2.6] ipv4: more compliant RFC 3168 support

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

Le dimanche 15 mai 2011 à 18:08 +0300, Stefanos Harhalakis a écrit :
> Hello,
> 
> On Sunday 15 of May 2011, Eric Dumazet wrote:
> > +static inline int ip4_frag_ecn_fold(u8 ecn)
> > +{
> > +	switch (ecn) {
> > +	/* If same ECN combination was observed on all fragments, do nothing */
> > +	case IPFRAG_ECN_NOT_ECT:
> > +	case IPFRAG_ECN_ECT_1:
> > +	case IPFRAG_ECN_ECT_0:
> > +	case IPFRAG_ECN_CE:
> > +	/* if a ECT_1 ECT_0 combination was observed, do nothing as well */
> > +	case IPFRAG_ECN_ECT_0 | IPFRAG_ECN_ECT_1:
> > +		return 0;
> > +	/* at least one fragment had CE, and others ECT_0 or ECT_1 */
> > +	case IPFRAG_ECN_CE | IPFRAG_ECN_ECT_0:
> > +	case IPFRAG_ECN_CE | IPFRAG_ECN_ECT_1:
> > +	case IPFRAG_ECN_CE | IPFRAG_ECN_ECT_0 | IPFRAG_ECN_ECT_1:
> > +		return INET_ECN_CE;
> > +	/* other combinations are invalid : drop frame */
> > +	default:
> > +		return -1;
> > +	}
> >  }
> 
> You may wish to simplify this exhaustive check to:
> 
> if (unlikely((ecn & IPFRAG_ECN_NOT_ECT) && ecn!=IPFRAG_ECN_NOT_ECT))
>   return -1;
> else if (ecn & IPFRAG_ECN_CE)
>   return INET_ECN_CE;
> else
>   return 0;
> 
> although I'm not sure which method will be faster.
> 

Problem of this version is that common frames in the Internet (NOT_ECT
or ECT_X or ECT_X) will take the longest path to come to "return 0;"

a switch() version is fast because gcc emits a table based jump

> Also, returning the exact same value for NOT_ECT and ECT_X and then ORing
> this with the TOS seems like it will make it loose the ECT_X info. No? (but
> also, I'm not sure if this is needed anyway from that point on).
> 

I dont understand what you mean here. We really need to not loose ECT_X,
and I believe we dont.

-1 : Drop the frame anyway
0 : No change on iph->tos field (we keep its value. it can have ECT_X.
Remember all fragments share same (iph->tos & 3) value)
3 : We make sure iph->tos is ORed with 3 to assert CE on result frame.

> p.s. I'm not sure whether this message will make it to the netdev list.

It should, no worry.