From owner-freebsd-hackers@FreeBSD.ORG Fri Oct 15 03:56:39 2010 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4FF61106566B; Fri, 15 Oct 2010 03:56:39 +0000 (UTC) (envelope-from yanegomi@gmail.com) Received: from mail-iw0-f182.google.com (mail-iw0-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id 04D1E8FC18; Fri, 15 Oct 2010 03:56:38 +0000 (UTC) Received: by iwn8 with SMTP id 8so431550iwn.13 for ; Thu, 14 Oct 2010 20:56:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=zIQce4TWq+cEO9O30RG3Uux9T17I2GBSi6IBXAww7Go=; b=TpAQoCAMSJpIhkblkwdGyzMUrZm1NXDzjGI8GD6tMm+lF8X7Y83fAjMaeKOmDNHuDS VXGsr+2lA/loiMhDdPPgCeKgpj+nhvKhJ8q5YMrSyOldvHuTQXfv0SHDt9l5Gz17HMEE xrzJRHEHIG8RMvYZJN3gtyWIbNyabvlABre9c= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=kpKliajeFbK0gttfsRw8KzBSD4Gbd4XaPD5GWZ5jmMxSEXW4YqKevFGmnhYfkH6SiN wf2NEuiluKQq4pRy1OwnC9hoIcjFI0SKtVe4GFniTy9ajn0HvD38YMrIJQjiRujsKbKh 4G7EWs15pKm0nzwmDwA/+oXEftuyOXJ7tzSws= MIME-Version: 1.0 Received: by 10.231.146.141 with SMTP id h13mr126868ibv.1.1287114563659; Thu, 14 Oct 2010 20:49:23 -0700 (PDT) Sender: yanegomi@gmail.com Received: by 10.231.184.3 with HTTP; Thu, 14 Oct 2010 20:49:23 -0700 (PDT) In-Reply-To: <201010140937.51953.jhb@freebsd.org> References: <4CB6F068.1070406@icyb.net.ua> <201010140937.51953.jhb@freebsd.org> Date: Thu, 14 Oct 2010 20:49:23 -0700 X-Google-Sender-Auth: sNKERK22M7adntGSGNdlRGafKJ8 Message-ID: From: Garrett Cooper To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-hackers@freebsd.org, Andriy Gapon Subject: Re: [PATCH] Bug with powerof2 macro in sys/param.h X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Oct 2010 03:56:39 -0000 On Thu, Oct 14, 2010 at 6:37 AM, John Baldwin wrote: > On Thursday, October 14, 2010 7:58:32 am Andriy Gapon wrote: >> on 14/10/2010 00:30 Garrett Cooper said the following: >> > =A0 =A0 I was talking to someone today about this macro, and he noted = that >> > the algorithm is incorrect -- it fails the base case with ((x) =3D=3D = 0 -- >> > which makes sense because 2^(x) cannot equal 0 (mathematically >> > impossible, unless you consider the limit as x goes to negative >> > infinity as log (0) / log(2) is undefined). I tested out his claim and >> > he was right: >> >> That's kind of obvious given the code. >> I think that this might be an intentional optimization. >> I guess that it doesn't really make sense to apply powerof2 to zero and = the users >> of the macro should do the check on their own if they expect zero as inp= ut (many >> places in the do not allow that). But the point is that this could be micro-optimizing things incorrectly. I'm running simple iteration tests to see what the performance is like, but the runtime is going to take a while to produce stable results. Mathematically there is a conflict with the definition of the macro, so it might confuse folks who pay attention to the math as opposed to the details (if you want I'll gladly add a comment around the macro in a patch to note the caveats of using powerof2). > I agree, the current macro is this way on purpose (and straight out of > "Hacker's Delight"). > > Of the existing calls you weren't sure of: > > sys/dev/cxgb/cxgb_sge.c: =A0 =A0 =A0 while (!powerof2(fl_q_size)) > sys/dev/cxgb/cxgb_sge.c: =A0 =A0 =A0 while (!powerof2(jumbo_q_size)) > > These are fine, will not be zero. Good to know. > sys/x86/x86/local_apic.c: =A0 =A0 =A0KASSERT(powerof2(count), ("bad count= ")); > sys/x86/x86/local_apic.c: =A0 =A0 =A0KASSERT(powerof2(align), ("bad align= ")); > > These are fine. =A0No code allocates zero IDT vectors. =A0We never alloca= te IDT > vectors for unallocated MSI or MSI-X IRQs. Excellent. > sys/net/flowtable.c: =A0 =A0 =A0 =A0 =A0 ft->ft_lock_count =3D > 2*(powerof2(mp_maxid + 1) ? (mp_maxid + 1): > > Clearly, 'mp_maxid + 1' will not be zero (barring a bizarre overflow case > which will not happen until we support 2^32 CPUs), so this is fine. But that should be caught by the mp_machdep code, correct? > sys/i386/pci/pci_pir.c: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (error && > !powerof2(pci_link->pl_irqmask)) { > > This fine. =A0Earlier in the function if pl_irqmask is zero, then all of = the > pci_pir_choose_irq() calls will fail, so this is only invoked if pl_irqma= sk > is non-zero. =A0In practice pl_irqmask is never zero anyway. Ok. > I suspect the GEOM ones are also generally safe. Well, that is the overall feeling I was getting, but I'm not sure that they're ok 100% of the time. What about the other patches? The mfiutil and mptutil ones at least get the two beforementioned tools in sync with sys/param.h at least, so I see some degree of value in the patches (even if they're just cleanup). Thanks, -Garrett