* [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
@ 2017-09-06 19:57 Arnd Bergmann
2017-09-06 20:22 ` Vishwanath Pai
2017-09-07 14:16 ` Geert Uytterhoeven
0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-09-06 19:57 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, David S. Miller
Cc: Arnd Bergmann, Vishwanath Pai, Josh Hunt, netfilter-devel,
coreteam, netdev, linux-kernel
64-bit division is expensive on 32-bit architectures, and
requires a special function call to avoid a link error like:
net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
In the case of hashlimit_mt_common, we don't actually need a
64-bit operation, we can simply rewrite the function slightly
to make that clear to the compiler.
Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
net/netfilter/xt_hashlimit.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 10d48234f5f4..50b53d86eef5 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
{
u64 r;
- r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
+ if (user > 0xFFFFFFFFULL)
+ return 0;
+
+ r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
r = (r - 1) << 4;
return r;
}
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
2017-09-06 19:57 [PATCH] netfilter: xt_hashlimit: avoid 64-bit division Arnd Bergmann
@ 2017-09-06 20:22 ` Vishwanath Pai
2017-09-06 20:48 ` Arnd Bergmann
2017-09-07 14:16 ` Geert Uytterhoeven
1 sibling, 1 reply; 6+ messages in thread
From: Vishwanath Pai @ 2017-09-06 20:22 UTC (permalink / raw)
To: Arnd Bergmann, Pablo Neira Ayuso, Jozsef Kadlecsik,
Florian Westphal, David S. Miller
Cc: Josh Hunt, netfilter-devel, coreteam, netdev, linux-kernel
On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
> 64-bit division is expensive on 32-bit architectures, and
> requires a special function call to avoid a link error like:
>
> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>
> In the case of hashlimit_mt_common, we don't actually need a
> 64-bit operation, we can simply rewrite the function slightly
> to make that clear to the compiler.
>
> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> net/netfilter/xt_hashlimit.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 10d48234f5f4..50b53d86eef5 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
> {
> u64 r;
>
> - r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
> + if (user > 0xFFFFFFFFULL)
> + return 0;
> +
> + r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
> r = (r - 1) << 4;
> return r;
> }
>
I have submitted another patch to fix this:
https://patchwork.ozlabs.org/patch/809881/
We have seen this problem before, I was careful not to introduce this
again in the new patch but clearly I overlooked this particular line :(
In the other cases we fixed it by replacing division with div64_u64().
-Vishwanath
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
2017-09-06 20:22 ` Vishwanath Pai
@ 2017-09-06 20:48 ` Arnd Bergmann
2017-09-07 10:19 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-09-06 20:48 UTC (permalink / raw)
To: Vishwanath Pai
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, Josh Hunt, netfilter-devel, coreteam,
Networking, Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 10:22 PM, Vishwanath Pai <vpai@akamai.com> wrote:
> On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
>> 64-bit division is expensive on 32-bit architectures, and
>> requires a special function call to avoid a link error like:
>>
>> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
>> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>>
>> In the case of hashlimit_mt_common, we don't actually need a
>> 64-bit operation, we can simply rewrite the function slightly
>> to make that clear to the compiler.
>>
>> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> net/netfilter/xt_hashlimit.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
>> index 10d48234f5f4..50b53d86eef5 100644
>> --- a/net/netfilter/xt_hashlimit.c
>> +++ b/net/netfilter/xt_hashlimit.c
>> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
>> {
>> u64 r;
>>
>> - r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
>> + if (user > 0xFFFFFFFFULL)
>> + return 0;
>> +
>> + r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
>> r = (r - 1) << 4;
>> return r;
>> }
>>
>
> I have submitted another patch to fix this:
> https://patchwork.ozlabs.org/patch/809881/
>
> We have seen this problem before, I was careful not to introduce this
> again in the new patch but clearly I overlooked this particular line :(
>
> In the other cases we fixed it by replacing division with div64_u64().
div64_u64() seems needlessly expensive here since the dividend
is known to be a 32-bit number. I guess the function is not called
frequently though, so it doesn't matter much.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
2017-09-06 20:48 ` Arnd Bergmann
@ 2017-09-07 10:19 ` Pablo Neira Ayuso
2017-09-07 11:16 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2017-09-07 10:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vishwanath Pai, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, Josh Hunt, netfilter-devel, coreteam,
Networking, Linux Kernel Mailing List
On Wed, Sep 06, 2017 at 10:48:22PM +0200, Arnd Bergmann wrote:
> On Wed, Sep 6, 2017 at 10:22 PM, Vishwanath Pai <vpai@akamai.com> wrote:
> > On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
> >> 64-bit division is expensive on 32-bit architectures, and
> >> requires a special function call to avoid a link error like:
> >>
> >> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
> >> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
> >>
> >> In the case of hashlimit_mt_common, we don't actually need a
> >> 64-bit operation, we can simply rewrite the function slightly
> >> to make that clear to the compiler.
> >>
> >> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> ---
> >> net/netfilter/xt_hashlimit.c | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> >> index 10d48234f5f4..50b53d86eef5 100644
> >> --- a/net/netfilter/xt_hashlimit.c
> >> +++ b/net/netfilter/xt_hashlimit.c
> >> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
> >> {
> >> u64 r;
> >>
> >> - r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
> >> + if (user > 0xFFFFFFFFULL)
> >> + return 0;
> >> +
> >> + r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
> >> r = (r - 1) << 4;
> >> return r;
> >> }
> >>
> >
> > I have submitted another patch to fix this:
> > https://patchwork.ozlabs.org/patch/809881/
> >
> > We have seen this problem before, I was careful not to introduce this
> > again in the new patch but clearly I overlooked this particular line :(
> >
> > In the other cases we fixed it by replacing division with div64_u64().
>
> div64_u64() seems needlessly expensive here since the dividend
> is known to be a 32-bit number. I guess the function is not called
> frequently though, so it doesn't matter much.
This is called from the packet path, only for the first packet for
each new destination IP entry in the hashtable, still from the
datapath. So if we can take something faster (for 32 bit arches) that
is correct, I think it's sensible to take.
Let me know in any case.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
2017-09-07 10:19 ` Pablo Neira Ayuso
@ 2017-09-07 11:16 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-09-07 11:16 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Vishwanath Pai, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, Josh Hunt, netfilter-devel, coreteam,
Networking, Linux Kernel Mailing List
On Thu, Sep 7, 2017 at 12:19 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Sep 06, 2017 at 10:48:22PM +0200, Arnd Bergmann wrote:
>> On Wed, Sep 6, 2017 at 10:22 PM, Vishwanath Pai <vpai@akamai.com> wrote:
>> > On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
>> >> 64-bit division is expensive on 32-bit architectures, and
>> >> requires a special function call to avoid a link error like:
>> >>
>> >> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
>> >> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>> >>
>> >> In the case of hashlimit_mt_common, we don't actually need a
>> >> 64-bit operation, we can simply rewrite the function slightly
>> >> to make that clear to the compiler.
>> >>
>> >> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
>> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >> ---
>> >> net/netfilter/xt_hashlimit.c | 5 ++++-
>> >> 1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
>> >> index 10d48234f5f4..50b53d86eef5 100644
>> >> --- a/net/netfilter/xt_hashlimit.c
>> >> +++ b/net/netfilter/xt_hashlimit.c
>> >> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
>> >> {
>> >> u64 r;
>> >>
>> >> - r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
>> >> + if (user > 0xFFFFFFFFULL)
>> >> + return 0;
>> >> +
>> >> + r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
>> >> r = (r - 1) << 4;
>> >> return r;
>> >> }
>> >>
>> >
>> > I have submitted another patch to fix this:
>> > https://patchwork.ozlabs.org/patch/809881/
>> >
>> > We have seen this problem before, I was careful not to introduce this
>> > again in the new patch but clearly I overlooked this particular line :(
>> >
>> > In the other cases we fixed it by replacing division with div64_u64().
>>
>> div64_u64() seems needlessly expensive here since the dividend
>> is known to be a 32-bit number. I guess the function is not called
>> frequently though, so it doesn't matter much.
>
> This is called from the packet path, only for the first packet for
> each new destination IP entry in the hashtable, still from the
> datapath. So if we can take something faster (for 32 bit arches) that
> is correct, I think it's sensible to take.
>
> Let me know in any case.
I think my version should be slightly better then, unless someone
finds something wrong with it.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
2017-09-06 19:57 [PATCH] netfilter: xt_hashlimit: avoid 64-bit division Arnd Bergmann
2017-09-06 20:22 ` Vishwanath Pai
@ 2017-09-07 14:16 ` Geert Uytterhoeven
1 sibling, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2017-09-07 14:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, Vishwanath Pai, Josh Hunt, netfilter-devel,
coreteam, netdev, linux-kernel
Hi Arnd,
On Wed, Sep 6, 2017 at 9:57 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 64-bit division is expensive on 32-bit architectures, and
> requires a special function call to avoid a link error like:
>
> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>
> In the case of hashlimit_mt_common, we don't actually need a
> 64-bit operation, we can simply rewrite the function slightly
> to make that clear to the compiler.
>
> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Thanks, this fixes a similar issue (__udivdi3 undefined) on m68k.
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-07 14:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 19:57 [PATCH] netfilter: xt_hashlimit: avoid 64-bit division Arnd Bergmann
2017-09-06 20:22 ` Vishwanath Pai
2017-09-06 20:48 ` Arnd Bergmann
2017-09-07 10:19 ` Pablo Neira Ayuso
2017-09-07 11:16 ` Arnd Bergmann
2017-09-07 14:16 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).