Re: [Qemu-devel] [RFC PATCH v2 22/23] spapr: Support ibm, dynamic-reconfiguration-memory

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3194 bytes --]

On Mon, Mar 30, 2015 at 02:41:50PM +0530, Bharata B Rao wrote:
> On Thu, Mar 26, 2015 at 02:44:17PM +1100, David Gibson wrote:
> > > +/*
> > > + * TODO: Take care of sparsemem configuration ?
> > > + */
> > > +static uint64_t numa_node_end(uint32_t nodeid)
> > > +{
> > > +    uint32_t i = 0;
> > > +    uint64_t addr = 0;
> > > +
> > > +    do {
> > > +        addr += numa_info[i].node_mem;
> > > +    } while (++i <= nodeid);
> > > +
> > > +    return addr;
> > > +}
> > > +
> > > +static uint64_t numa_node_start(uint32_t nodeid)
> > > +{
> > > +    if (!nodeid) {
> > > +        return 0;
> > > +    } else {
> > > +        return numa_node_end(nodeid - 1);
> > > +    }
> > > +}
> > > +
> > > +/*
> > > + * Given the addr, return the NUMA node to which the address belongs to.
> > > + */
> > > +static uint32_t get_numa_node(uint64_t addr)
> > > +{
> > > +    uint32_t i;
> > > +
> > > +    for (i = 0; i < nb_numa_nodes; i++) {
> > > +        if ((addr >= numa_node_start(i)) && (addr < numa_node_end(i))) {
> > > +            return i;
> > > +        }
> > > +    }
> > 
> > This function is O(N^2) in number of nodes, which is a bit hideous for
> > something so simple.
> 
> Will something like below work ? Will all archs be ok with this ?

The below looks like a reasonable idea to me, but it's really the call
of the numa and memory subsystem people.

Paolo, do you have an opinion?  The basic problem here is to have a
function which returns which NUMA node a given address lies in.

> numa: Store start and end address range of each node in numa_info
> 
> Keep track of start and end address of each NUMA node in numa_info
> structure so that lookup of node by address becomes easier.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  include/sysemu/numa.h |    3 +++
>  numa.c                |   28 ++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 5633b85..6dd6387 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -14,11 +14,14 @@ typedef struct node_info {
>      DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
>      struct HostMemoryBackend *node_memdev;
>      bool present;
> +    uint64_t mem_start;
> +    uint64_t mem_end;

I suspect these should be hwaddr, or possible ram_addr_t though.

[snip]
> > I don't think rtas_ld() should be used outside its intended function
> > in rtas.  Make a direct call to ldl_phys instead.
> 
> Ok, but @table here is an RTAS arg passed from the main RTAS routine to this
> function. Still can't use rtas_ld() ?

So, rtas_ld() was never intended as a general purpose load function
for RTAS use, but specifically for loading arguments from an RTAS
style argument list.

It's already being used for more than that now in the RTAS code, which
I'm not entirely comfortable with.  I'd prefer not to extend its use
any further.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]