Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755992AbdIGSnN (ORCPT ); Thu, 7 Sep 2017 14:43:13 -0400 Received: from mail-it0-f41.google.com ([209.85.214.41]:36563 "EHLO mail-it0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754650AbdIGSnH (ORCPT ); Thu, 7 Sep 2017 14:43:07 -0400 X-Google-Smtp-Source: ADKCNb62EzKE5LkXB8zQ2pzSa36Hj9edXy4pgUUtYFVLLhKaA2NYAoEQAQE/7u1K2Fyc1sL98PNk7s4f185seDwr7c0= MIME-Version: 1.0 In-Reply-To: <69b38985-e094-ddcc-6f7e-d6e5cc2c657e@akamai.com> References: <69b38985-e094-ddcc-6f7e-d6e5cc2c657e@akamai.com> From: Linus Torvalds Date: Thu, 7 Sep 2017 11:43:06 -0700 X-Google-Sender-Auth: 9Sbx2mT5ALffm4wMr8pBuCIrSKU Message-ID: Subject: Re: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove the restore_c_regs_and_iret label) To: Vishwanath Pai Cc: Ingo Molnar , Igor Lubashev , Josh Hunt , Pablo Neira Ayuso , Borislav Petkov , Andy Lutomirski , "the arch/x86 maintainers" , Linux Kernel Mailing List , Brian Gerst , Andrew Cooper , Juergen Gross , Boris Ostrovsky , Kees Cook , Andrew Morton , "David S. Miller" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1408 Lines: 44 On Thu, Sep 7, 2017 at 11:25 AM, Vishwanath Pai wrote: > On 09/07/2017 01:51 PM, Linus Torvalds wrote: >> >> But honestly, that math is odd in other ways too (is that "r-1" >> _supposed_ to underflow to -1 for large 'user' counts?), so somebody >> needs to look at that logic. > > Sorry about the build failure, we have already queued up a fix for this: > https://patchwork.ozlabs.org/patch/810772/ 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