From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:40714)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ehabkost@redhat.com>) id 1X1jRw-0001FP-Lk
	for qemu-devel@nongnu.org; Mon, 30 Jun 2014 17:49:26 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <ehabkost@redhat.com>) id 1X1jRq-0007K0-Gy
	for qemu-devel@nongnu.org; Mon, 30 Jun 2014 17:49:20 -0400
Received: from mx1.redhat.com ([209.132.183.28]:26761)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ehabkost@redhat.com>) id 1X1jRq-0007Jk-8n
	for qemu-devel@nongnu.org; Mon, 30 Jun 2014 17:49:14 -0400
Date: Mon, 30 Jun 2014 18:48:59 -0300
From: Eduardo Habkost <ehabkost@redhat.com>
Message-ID: <20140630214859.GP3222@otherpad.lan.raisama.net>
References: <20140623193310.GD4323@linux.vnet.ibm.com>
	<20140624004825.GE4323@linux.vnet.ibm.com>
	<20140624174038.GK4323@linux.vnet.ibm.com>
	<20140626193705.GJ3222@otherpad.lan.raisama.net>
	<20140630182610.GA15697@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140630182610.GA15697@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [RFC PATCH v3] numa: enable sparse node numbering
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>, Alexey Kardashevskiy <aik@ozlabs.ru>, Hu Tao <hutao@cn.fujitsu.com>, qemu-devel@nongnu.org, Anton Blanchard <anton@samba.org>, David Rientjes <rientjes@google.com>, Igor Mammedov <imammedo@redhat.com>

On Mon, Jun 30, 2014 at 11:26:10AM -0700, Nishanth Aravamudan wrote:
[...]
> > > -        if (i == nb_numa_nodes) {
> > > +        if (i == max_numa_node) {
> > >              for (i = 0; i < max_cpus; i++) {
> > > -                set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
> > > +                do {
> > > +                    j = (j + 1) % max_numa_node;
> > > +                } while (!numa_info[j].present);
> > 
> > If you change it from "do { } while" to "while { }", you don't need to
> > initialize j to -1.
> 
> I don't think that's quite as simple as you make it out to be. If you
> use a while() loop, we won't always increment j, which means once we've
> found a present node, we'll always use that node? j here basically
> represents the *last* used nodeid, which we don't want to use again when
> we re-enter the for-loop, we want to use the next present nodeid. It
> seems like the do {} while() does this fine?  I could use a while() if I
> added another increment outside the loop, as follows:
> 
>         if (i == max_numa_nodeid) {
>             for (i = 0, j = 0; i < max_cpus; i++) {
>                 while (!numa_info[j].present) {
>                     j = (j + 1) % (max_numa_nodeid);
>                 }
>                 set_bit(i, numa_info[j].node_cpu);
>                 j = (j + 1) % (max_numa_nodeid);
>             }
>         }
> 
> If you think that is cleaner, I'll use that version.

You are right, and your "do { } while" version looks cleaner than
duplicating the "j = (j + 1) % (max_numa_nodeid)" line.

-- 
Eduardo