lkml.org 
[lkml]   [2017]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove the restore_c_regs_and_iret label)
    From
    Date
    On 09/07/2017 02:43 PM, Linus Torvalds wrote:
    > Note: that patch has *exactly* the issue I was talking about above.
    >
    > Doing that
    >
    > if (user > 0xFFFFFFFFULL)
    > return 0;
    >
    > is different from the old code, which used to result in a zero in the
    > divide, and then
    >
    > r = (r - 1) << 4;
    >
    > would cause it to return a large value.
    >
    > So the patch in question doesn't just fix the build error, it
    > completely changes the semantics of the function too.
    >
    > I *think* the new behavior is likely what you want, but these kinds of
    > things should be _described_.
    >
    > Also, even with the patch, we have garbage:
    >
    > 0xFFFFFFFFULL / (u32)user
    >
    > why is that sub-expression pointlessly doing a 64-bit divide with a
    > 32-bit number? The compiler is hopefully smart enough to point things
    > out, but that "ULL" really is _wrong_ there, and could cause a stupid
    > compiler to still do a 64-bit divide (although hopefully the simpler
    > version that is 64/32).
    >
    > So please clarify both the correct behavior _and_ the actual typing of
    > the divide, ok?
    >
    > Linus

    The value of 'user' is sent from userspace, which is the return value of
    this function:

    static uint64_t bytes_to_cost(uint32_t bytes)
    {
    uint32_t r = bytes >> XT_HASHLIMIT_BYTE_SHIFT;
    return UINT32_MAX / (r+1);
    }

    What user2rate_bytes() is trying to do is the opposite of above. The
    size of 'user' is 64bit for a different reason altogether, but in this
    case it is guaranteed to be always < U32_MAX. And hence using 64bit
    divide is completely pointless (which I now realize).

    Writing U32INT_MAX as 0xFFFFFFFFULL was a mistake on my part. I could
    have avoided all of this by using built-in constants instead of trying
    to define them myself. I will rewrite the function as below and send out
    another patch:

    static u64 user2rate_bytes(u64 user)
    {
    u64 r;

    r = user ? U32_MAX / (u32) user : U32_MAX;
    r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
    return r;
    }

    -Vishwanath

    \
     
     \ /
      Last update: 2017-09-07 22:18    [W:9.909 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site