From owner-svn-src-user@FreeBSD.ORG Mon May 13 18:43:14 2013 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 82F0E9F8; Mon, 13 May 2013 18:43:14 +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 59703163; Mon, 13 May 2013 18:43:14 +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 9D9B9B917; Mon, 13 May 2013 14:43:12 -0400 (EDT) Message-ID: <51913443.1060600@FreeBSD.org> Date: Mon, 13 May 2013 14:43:15 -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> 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 14:43:12 -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 18:43:14 -0000 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. -- John Baldwin