Re: [PATCH] netfilter: xtables: introduce xt_length revision 2½

[Patrick McHardy <kaber@trash.net>]

On 24.07.2010 14:27, Jan Engelhardt wrote:
> 
> On Saturday 2010-07-24 13:42, Eric Dumazet wrote:
>>> +static bool
>>> +length2_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>> +{
>>> +	const struct xt_length_mtinfo2 *info = par->matchinfo;
>>> +	const struct iphdr *iph = ip_hdr(skb);
>>> +	unsigned int len = 0;
>>> +	bool hit = true;
>>> +
>>> +	if (info->flags & XT_LENGTH_LAYER3)
>>> +		len = ntohs(iph->tot_len);
>>> +	else if (info->flags & XT_LENGTH_LAYER4)
>>> +		len = ntohs(iph->tot_len) - par->thoff;
>>> +	else if (info->flags & XT_LENGTH_LAYER5)
>>> +		hit = xtlength_layer5(&len, skb, iph->protocol, par->thoff);
>>> +	else if (info->flags & XT_LENGTH_LAYER7)
>>> +		hit = xtlength_layer7(&len, skb, iph->protocol, par->thoff);
>>> +	if (!hit)
>>> +		return false;
>>> +
>>> +	return (len >= info->min && len <= info->max) ^
>>> +	       !!(info->flags & XT_LENGTH_INVERT);
>>> +}
>>
>>
>> This serie of tests is expensive and useless.
>>
>> - A switch() would be faster,
>>  - if you dont use a bit mask, but continuous values to get the layer.
>> - Also, using a u16 is more expensive than a u32.
>> - On x86, compiler is forced to use prefixes or conversions instructions
>>  (movzwl), this makes code bigger.
> 
> I might agree with u16/u32 in that some CPUs need to run an extra
> "AND" when they don't have (movw/cmpw), but... the other things
> cast my doubts.
> 
> C does not specify a "speed" for switch or if. I agree that some
> constructs may desire to be reworked to work around limitations of
> the compiler's smartness, but in the case that is before us, it looks
> like some of your arguments have no base - at least on x86_64.
> 
> length2_mt as it stands (bitmask, u16, if): 800 bytes
> length2_mt with u32 flag: 800 bytes
> length2_mt with switch: 800 bytes
> length2_mt with continuous values and u32: 816 bytes
> length2_mt with cont.v, u32, and switch: 816 bytes
> 
> The compiler is smart enough to see that a run of if tests against
> the same variable with different values is transformable into a
> switch statement.

They are mutually exclusive though, so using a bitmask doesn't make
much sense.