svn commit: r250546 - user/attilio/jeff-numa/sys/x86/acpica

[Attilio Rao]

On Mon, May 13, 2013 at 8:43 PM, John Baldwin <jhb at freebsd.org> wrote:
> On 5/13/13 2:21 PM, Attilio Rao wrote:
>> On Mon, May 13, 2013 at 7:52 PM, John Baldwin <jhb at freebsd.org> 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?

Attilio


--
Peace can only be achieved by understanding - A. Einstein