Re: [NETFILTER][PATCH] Re: Question about the hashlimit network mask patch

[Patrick McHardy <kaber@trash.net>]

Jan Engelhardt wrote:
> On Tuesday 2008-04-01 14:49, Patrick McHardy wrote:
> 
>> Jan Engelhardt wrote:
>>>  [NETFILTER]: xt_hashlimit: add workaround for >>32 case
>>>
>>>  Hardware surprisingly does nothing when a 32-bit right-shift is
>>>  to be done. Worse yet, compilers do not even work around it.
>>
>> Thats because the C standard states that the result is undefined.
>> Anyways, I think this patch is slightly nicer because it
>> gets rid of the double negation and the %32 == 0 special-casing
>> for IPv6.
>>
>> Do you want to add an ACKed-by?
> 
> 
> The special casing for %32==0 in the IPv6 block was not a workaround for 
> '>>32', but speed. 

I'm aware of that.

> Assuming the C code is already perfect and gets 
> transformed 1:1 into assembly without any further optimization, the cost 
> of the operation is 4 simple 'mov' instructions for %32==0, whereas 
> calling maskl() will involve a fair number of operations (at least 27 by 
> the count). [With your patch, maskl() is now 28 + a branch.]

The "optimization" results in bigger code and more branches itself.
Just removing it without fixing maskl reduces code size by over 35%:

net/netfilter/xt_hashlimit.c:
   hashlimit_ipv6_mask | -103 # 274 -> 171
  1 function changed, 103 bytes removed

Fixing maskl bloats it up again, but still a net win:

   hashlimit_ipv6_mask |   -9 # 274 -> 265, size inlines: 71 -> 148
   hashlimit_init_dst  |   +6 # 589 -> 595, size inlines: 86 -> 106
  2 functions changed, 6 bytes added, 9 bytes removed, diff: -3

Just the maskl fix results in:

net/netfilter/xt_hashlimit.c:
   hashlimit_ipv6_mask |  +95 # 274 -> 369, size inlines: 71 -> 143
   hashlimit_init_dst  |   +6 # 589 -> 595, size inlines: 86 -> 106
  2 functions changed, 101 bytes added