From owner-svn-src-user@FreeBSD.ORG Mon May 13 19:14:02 2013 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 1AAAACF2; Mon, 13 May 2013 19:14:02 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) by mx1.freebsd.org (Postfix) with ESMTP id E3F46301; Mon, 13 May 2013 19:14:01 +0000 (UTC) Received: from dhcp-10-2-212-236.hudson-trading.com (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 5A5B0B918; Mon, 13 May 2013 15:14:01 -0400 (EDT) Message-ID: <51913B7D.6040805@FreeBSD.org> Date: Mon, 13 May 2013 15:14:05 -0400 From: John Baldwin User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/20130328 Thunderbird/17.0.5 MIME-Version: 1.0 To: attilio@FreeBSD.org Subject: Re: svn commit: r250546 - user/attilio/jeff-numa/sys/x86/acpica References: <201305120210.r4C2AFpt076600@svn.freebsd.org> <51912863.6010601@FreeBSD.org> <51913443.1060600@FreeBSD.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 13 May 2013 15:14:01 -0400 (EDT) Cc: src-committers@freebsd.org, svn-src-user@freebsd.org X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 May 2013 19:14:02 -0000 On 5/13/13 2:54 PM, Attilio Rao wrote: > On Mon, May 13, 2013 at 8:43 PM, John Baldwin wrote: >> On 5/13/13 2:21 PM, Attilio Rao wrote: >>> On Mon, May 13, 2013 at 7:52 PM, John Baldwin wrote: >>>> On 5/11/13 10:10 PM, Attilio Rao wrote: >>>>> Author: attilio >>>>> Date: Sun May 12 02:10:15 2013 >>>>> New Revision: 250546 >>>>> URL: http://svnweb.freebsd.org/changeset/base/250546 >>>>> >>>>> Log: >>>>> Add the code to setup the correct number of probed memory domains. >>>>> >>>>> Sponsored by: EMC / Isilon storage division >>>>> Obtained from: jeff >>>>> >>>>> Modified: >>>>> user/attilio/jeff-numa/sys/x86/acpica/srat.c >>>>> >>>>> Modified: user/attilio/jeff-numa/sys/x86/acpica/srat.c >>>>> ============================================================================== >>>>> --- user/attilio/jeff-numa/sys/x86/acpica/srat.c Sun May 12 01:58:04 2013 (r250545) >>>>> +++ user/attilio/jeff-numa/sys/x86/acpica/srat.c Sun May 12 02:10:15 2013 (r250546) >>>>> @@ -244,33 +244,34 @@ static int >>>>> renumber_domains(void) >>>>> { >>>>> int domains[VM_PHYSSEG_MAX]; >>>>> - int ndomain, i, j, slot; >>>>> + int i, j, slot; >>>>> >>>>> /* Enumerate all the domains. */ >>>>> - ndomain = 0; >>>>> + vm_ndomain = 0; >>>>> for (i = 0; i < num_mem; i++) { >>>>> /* See if this domain is already known. */ >>>>> - for (j = 0; j < ndomain; j++) { >>>>> + for (j = 0; j < vm_ndomain; j++) { >>>>> if (domains[j] >= mem_info[i].domain) >>>>> break; >>>>> } >>>>> - if (j < ndomain && domains[j] == mem_info[i].domain) >>>>> + if (j < vm_ndomain && domains[j] == mem_info[i].domain) >>>>> continue; >>>>> >>>>> /* Insert the new domain at slot 'j'. */ >>>>> slot = j; >>>>> - for (j = ndomain; j > slot; j--) >>>>> + for (j = vm_ndomain; j > slot; j--) >>>>> domains[j] = domains[j - 1]; >>>>> domains[slot] = mem_info[i].domain; >>>>> - ndomain++; >>>>> - if (ndomain > MAXMEMDOM) { >>>>> + vm_ndomain++; >>>>> + if (vm_ndomain > MAXMEMDOM) { >>>>> + vm_ndomain = 1; >>>>> printf("SRAT: Too many memory domains\n"); >>>>> return (EFBIG); >>>>> } >>>>> } >>>>> >>>>> /* Renumber each domain to its index in the sorted 'domains' list. */ >>>>> - for (i = 0; i < ndomain; i++) { >>>>> + for (i = 0; i < vm_ndomain; i++) { >>>>> /* >>>>> * If the domain is already the right value, no need >>>>> * to renumber. >>>>> >>>> >>>> Why not just set vm_ndomain = ndomain at the end? The current code aims >>>> to only set the global variables if it successfully parses the entire >>>> table. Setting vm_ndomain at the end would be consistent with this >>>> model (and a much smaller diff). >>> >>> The provided diff is consistent. >>> It does reset to 1 (initial value) in case of error, otherwise it is >>> just the correct value (see the KASSERT in the end). >> >> That is not what I was saying. The previous code used a temporary >> variable while it built the affinity info and only set 'mem_affinity' at >> the end. It never had to back out and clear it back to NULL if there >> was an error (which is more error prone). Following the same model here >> would mean that you would leave vm_ndomains alone and only set it at the >> very end when mem_affinity is set after all checks have passed. > > I understand what you are saying, and I'm replying that your objection > is nosense because the effect will be just the same. > Are you seriously thinking that this version is less readable? Only that it is more error prone as you have to remember to explicitly back out if there is an error. -- John Baldwin