| Message ID | 20160930160559.4102745-3-arnd@arndb.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Fri, 2016-09-30 at 18:05 +0200, Arnd Bergmann wrote: > The newly added support for high-resolution pps rates introduced multiple 64-bit > division operations in one function, which fails on all 32-bit architectures: > > net/netfilter/xt_hashlimit.o: In function `user2credits': > xt_hashlimit.c:(.text.user2credits+0x3c): undefined reference to `__aeabi_uldivmod' > xt_hashlimit.c:(.text.user2credits+0x68): undefined reference to `__aeabi_uldivmod' > xt_hashlimit.c:(.text.user2credits+0x88): undefined reference to `__aeabi_uldivmod' > > This replaces the division with an explicit call to div_u64 for version 2 > to documents that this is a slow operation, and reverts back to 32-bit arguments > for the version 1 data to restore the original faster 32-bit division. > > With both changes combined, we no longer get a link error. > > Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to support higher pps rates") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Vishwanath Pai already sent a patch for this, and I did my version independently. > The difference is that his version also the more expensive division for the > version 1 variant that doesn't need it. > > See also http://patchwork.ozlabs.org/patch/676713/ > --- > net/netfilter/xt_hashlimit.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c > index 44a095ecc7b7..3d5525df6eb3 100644 > --- a/net/netfilter/xt_hashlimit.c > +++ b/net/netfilter/xt_hashlimit.c > @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len) > static u64 user2credits(u64 user, int revision) > { > if (revision == 1) { > + u32 user32 = user; /* use 32-bit division */ > + This looks dangerous to me. Have you really tried all possible cases ? Caller (even if using revision == 1) does user2credits(cfg->avg * cfg->burst, revision); Since this is not a fast path, I would prefer to keep the 64bit divide. Vishwanath version looks safer. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/30/2016 12:38 PM, Eric Dumazet wrote: > On Fri, 2016-09-30 at 18:05 +0200, Arnd Bergmann wrote: >> The newly added support for high-resolution pps rates introduced multiple 64-bit >> division operations in one function, which fails on all 32-bit architectures: >> >> net/netfilter/xt_hashlimit.o: In function `user2credits': >> xt_hashlimit.c:(.text.user2credits+0x3c): undefined reference to `__aeabi_uldivmod' >> xt_hashlimit.c:(.text.user2credits+0x68): undefined reference to `__aeabi_uldivmod' >> xt_hashlimit.c:(.text.user2credits+0x88): undefined reference to `__aeabi_uldivmod' >> >> This replaces the division with an explicit call to div_u64 for version 2 >> to documents that this is a slow operation, and reverts back to 32-bit arguments >> for the version 1 data to restore the original faster 32-bit division. >> >> With both changes combined, we no longer get a link error. >> >> Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to support higher pps rates") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> Vishwanath Pai already sent a patch for this, and I did my version independently. >> The difference is that his version also the more expensive division for the >> version 1 variant that doesn't need it. >> >> See also http://patchwork.ozlabs.org/patch/676713/ >> --- >> net/netfilter/xt_hashlimit.c | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c >> index 44a095ecc7b7..3d5525df6eb3 100644 >> --- a/net/netfilter/xt_hashlimit.c >> +++ b/net/netfilter/xt_hashlimit.c >> @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len) >> static u64 user2credits(u64 user, int revision) >> { >> if (revision == 1) { >> + u32 user32 = user; /* use 32-bit division */ >> + > > This looks dangerous to me. Have you really tried all possible cases ? > > Caller (even if using revision == 1) does > user2credits(cfg->avg * cfg->burst, revision); > It does look like we might lose precision here because of 64bit to 32bit conversion, but I am not sure how much it matters here. Iirc this is how it used to be before rev2 code. > Since this is not a fast path, I would prefer to keep the 64bit divide. > Agreed, this code does not get executed too often for us to worry about div_u64 being slow. And it reverts back to regular division on 64 bit arch anyways. > Vishwanath version looks safer. > > -Vishwanath -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 30 September 2016, Eric Dumazet wrote: > On Fri, 2016-09-30 at 18:05 +0200, Arnd Bergmann wrote: > > net/netfilter/xt_hashlimit.c | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c > > index 44a095ecc7b7..3d5525df6eb3 100644 > > --- a/net/netfilter/xt_hashlimit.c > > +++ b/net/netfilter/xt_hashlimit.c > > @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len) > > static u64 user2credits(u64 user, int revision) > > { > > if (revision == 1) { > > + u32 user32 = user; /* use 32-bit division */ > > + > > This looks dangerous to me. Have you really tried all possible cases ? Yes, I'm pretty certain about that: The 11d5f15723c9 patch that introduced this kept the existing implementation for the revision==1 case, except for changing the types. > Caller (even if using revision == 1) does > user2credits(cfg->avg * cfg->burst, revision); > > Since this is not a fast path, I would prefer to keep the 64bit divide. > > Vishwanath version looks safer. Ok, fair enough. I couldn't tell how much of a fast path this was, and it's more a general issue that I see with other developers blindly using div_u64() whenever getting this link error. Since I already had the patch by the time I saw the other one (which is also at v3 and got comments), I just sent it out along with the other two patches I had for netfilter. I also ended up introducing a typo in a last-minute change, so I'll let Vishwanath and you work out the best implementation and withdraw my version. Arnd -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c index 44a095ecc7b7..3d5525df6eb3 100644 --- a/net/netfilter/xt_hashlimit.c +++ b/net/netfilter/xt_hashlimit.c @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len) static u64 user2credits(u64 user, int revision) { if (revision == 1) { + u32 user32 = user; /* use 32-bit division */ + /* If multiplying would overflow... */ - if (user > 0xFFFFFFFF / (HZ*CREDITS_PER_JIFFY_v1)) + if (user32 > 0xFFFFFFFF / (HZ*CREDITS_PER_JIFFY_v1)) /* Divide first. */ - return (user / XT_HASHLIMIT_SCALE) *\ + return (user32 / XT_HASHLIMIT_SCALE) * HZ * CREDITS_PER_JIFFY_v1; - return (user * HZ * CREDITS_PER_JIFFY_v1) \ - / XT_HASHLIMIT_SCALE; + return (user32 * HZ * CREDITS_PER_JIFFY_v1) / + XT_HASHLIMIT_SCALE; } else { if (user > 0xFFFFFFFFFFFFFFFF / (HZ*CREDITS_PER_JIFFY)) - return (user / XT_HASHLIMIT_SCALE_v2) *\ - HZ * CREDITS_PER_JIFFY; + return div_u64_u64(user, XT_HASHLIMIT_SCALE_v2) * + HZ * CREDITS_PER_JIFFY; - return (user * HZ * CREDITS_PER_JIFFY) / XT_HASHLIMIT_SCALE_v2; + return div_u64_u64(user * HZ * CREDITS_PER_JIFFY, + XT_HASHLIMIT_SCALE_v2); } }
The newly added support for high-resolution pps rates introduced multiple 64-bit division operations in one function, which fails on all 32-bit architectures: net/netfilter/xt_hashlimit.o: In function `user2credits': xt_hashlimit.c:(.text.user2credits+0x3c): undefined reference to `__aeabi_uldivmod' xt_hashlimit.c:(.text.user2credits+0x68): undefined reference to `__aeabi_uldivmod' xt_hashlimit.c:(.text.user2credits+0x88): undefined reference to `__aeabi_uldivmod' This replaces the division with an explicit call to div_u64 for version 2 to documents that this is a slow operation, and reverts back to 32-bit arguments for the version 1 data to restore the original faster 32-bit division. With both changes combined, we no longer get a link error. Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to support higher pps rates") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- Vishwanath Pai already sent a patch for this, and I did my version independently. The difference is that his version also the more expensive division for the version 1 variant that doesn't need it. See also http://patchwork.ozlabs.org/patch/676713/ --- net/netfilter/xt_hashlimit.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)