Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On Tue, Jun 02, 2015 at 07:57:55AM +0100, Jan Beulich wrote:
On 02.06.15 at 08:35, chao.p.p...@linux.intel.com wrote:
On Fri, May 29, 2015 at 09:52:03AM +0100, Jan Beulich wrote:
On 29.05.15 at 10:28, chao.p.p...@linux.intel.com wrote:
On Fri, May 29, 2015 at 09:01:53AM +0100, Jan Beulich wrote:
On 29.05.15 at 04:35, chao.p.p...@linux.intel.com wrote:
On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int
max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores
*
+
boot_cpu_data.x86_num_siblings);
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus,
cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
You mean the declaration or invocation? If the former I have no
special
reason for it (e.g. I can change it).
Neither - I just don't see the need for a new function.
In which case the invocation of set_nr_cpu_ids() should move to the
place where now set_nr_sockets() is invoked, to make sure
boot_cpu_data.x86_max_cores/x86_num_siblings available, which may not be
your expectation.
Ah, in which case this _is_ the explanation, albeit only provided the
use of the two boot_cpu_data fields has to remain (which I had put
under question). And if these have to remain, couldn't this be done
in a presmp initcall instead of an explicitly called function?
presmp is too late. nr_sockets will get used in smp_prepare_cpus()
before calling set_cpu_sibling_map for cpu 0.
Okay. In which case - why not calculate the value there?
Okay, then I just need move the invocation of set_nr_sockets() from
__start_xen() to smp_prepare_cpus().
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On 02.06.15 at 08:35, chao.p.p...@linux.intel.com wrote:
On Fri, May 29, 2015 at 09:52:03AM +0100, Jan Beulich wrote:
On 29.05.15 at 10:28, chao.p.p...@linux.intel.com wrote:
On Fri, May 29, 2015 at 09:01:53AM +0100, Jan Beulich wrote:
On 29.05.15 at 04:35, chao.p.p...@linux.intel.com wrote:
On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores *
+
boot_cpu_data.x86_num_siblings);
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
You mean the declaration or invocation? If the former I have no special
reason for it (e.g. I can change it).
Neither - I just don't see the need for a new function.
In which case the invocation of set_nr_cpu_ids() should move to the
place where now set_nr_sockets() is invoked, to make sure
boot_cpu_data.x86_max_cores/x86_num_siblings available, which may not be
your expectation.
Ah, in which case this _is_ the explanation, albeit only provided the
use of the two boot_cpu_data fields has to remain (which I had put
under question). And if these have to remain, couldn't this be done
in a presmp initcall instead of an explicitly called function?
presmp is too late. nr_sockets will get used in smp_prepare_cpus()
before calling set_cpu_sibling_map for cpu 0.
Okay. In which case - why not calculate the value there?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On Fri, May 29, 2015 at 09:52:03AM +0100, Jan Beulich wrote:
On 29.05.15 at 10:28, chao.p.p...@linux.intel.com wrote:
On Fri, May 29, 2015 at 09:01:53AM +0100, Jan Beulich wrote:
On 29.05.15 at 04:35, chao.p.p...@linux.intel.com wrote:
On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores *
+
boot_cpu_data.x86_num_siblings);
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
You mean the declaration or invocation? If the former I have no special
reason for it (e.g. I can change it).
Neither - I just don't see the need for a new function.
In which case the invocation of set_nr_cpu_ids() should move to the
place where now set_nr_sockets() is invoked, to make sure
boot_cpu_data.x86_max_cores/x86_num_siblings available, which may not be
your expectation.
Ah, in which case this _is_ the explanation, albeit only provided the
use of the two boot_cpu_data fields has to remain (which I had put
under question). And if these have to remain, couldn't this be done
in a presmp initcall instead of an explicitly called function?
presmp is too late. nr_sockets will get used in smp_prepare_cpus()
before calling set_cpu_sibling_map for cpu 0.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On Fri, May 29, 2015 at 09:01:53AM +0100, Jan Beulich wrote:
On 29.05.15 at 04:35, chao.p.p...@linux.intel.com wrote:
On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores *
+ boot_cpu_data.x86_num_siblings);
How did you come to this expression for the bitmap size? I.e.
why not simply physids_weight(phys_cpu_present_map)?
physids_weight(phys_cpu_present_map) gives me cpus for all sockets.
While here the 'cpus' is actually _cpus_per_socket_. I used the max
possible cpus indicated in cpuid as the upper bound so bitmap_weight()
returns the actual available cpus on socket 0.
In which case the variable name is badly chosen, or a respective
comment is missing.
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
You mean the declaration or invocation? If the former I have no special
reason for it (e.g. I can change it).
Neither - I just don't see the need for a new function.
In which case the invocation of set_nr_cpu_ids() should move to the
place where now set_nr_sockets() is invoked, to make sure
boot_cpu_data.x86_max_cores/x86_num_siblings available, which may not be
your expectation.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On 29.05.15 at 04:35, chao.p.p...@linux.intel.com wrote:
On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores *
+ boot_cpu_data.x86_num_siblings);
How did you come to this expression for the bitmap size? I.e.
why not simply physids_weight(phys_cpu_present_map)?
physids_weight(phys_cpu_present_map) gives me cpus for all sockets.
While here the 'cpus' is actually _cpus_per_socket_. I used the max
possible cpus indicated in cpuid as the upper bound so bitmap_weight()
returns the actual available cpus on socket 0.
In which case the variable name is badly chosen, or a respective
comment is missing.
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
You mean the declaration or invocation? If the former I have no special
reason for it (e.g. I can change it).
Neither - I just don't see the need for a new function.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On 29.05.15 at 10:28, chao.p.p...@linux.intel.com wrote:
On Fri, May 29, 2015 at 09:01:53AM +0100, Jan Beulich wrote:
On 29.05.15 at 04:35, chao.p.p...@linux.intel.com wrote:
On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores *
+ boot_cpu_data.x86_num_siblings);
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
You mean the declaration or invocation? If the former I have no special
reason for it (e.g. I can change it).
Neither - I just don't see the need for a new function.
In which case the invocation of set_nr_cpu_ids() should move to the
place where now set_nr_sockets() is invoked, to make sure
boot_cpu_data.x86_max_cores/x86_num_siblings available, which may not be
your expectation.
Ah, in which case this _is_ the explanation, albeit only provided the
use of the two boot_cpu_data fields has to remain (which I had put
under question). And if these have to remain, couldn't this be done
in a presmp initcall instead of an explicitly called function?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores *
+ boot_cpu_data.x86_num_siblings);
How did you come to this expression for the bitmap size? I.e.
why not simply physids_weight(phys_cpu_present_map)?
physids_weight(phys_cpu_present_map) gives me cpus for all sockets.
While here the 'cpus' is actually _cpus_per_socket_. I used the max
possible cpus indicated in cpuid as the upper bound so bitmap_weight()
returns the actual available cpus on socket 0.
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
You mean the declaration or invocation? If the former I have no special
reason for it (e.g. I can change it).
+/* Representing HT and core siblings in each socket */
+extern cpumask_var_t *socket_cpumask;
Comment style.
Ah, stop is missing here.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/13] x86: add socket_cpumask
On 21.05.15 at 10:41, chao.p.p...@linux.intel.com wrote:
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
#endif
}
+void __init set_nr_sockets(void)
+{
+unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
+ boot_cpu_data.x86_max_cores *
+ boot_cpu_data.x86_num_siblings);
How did you come to this expression for the bitmap size? I.e.
why not simply physids_weight(phys_cpu_present_map)?
+
+if ( cpus == 0 )
+cpus = 1;
+
+nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
+}
Is there a reason why this can't just be added to the end of the
immediately preceding set_nr_cpu_ids()?
@@ -638,6 +649,8 @@ static int cpu_smpboot_alloc(unsigned int cpu)
unsigned int order, memflags = 0;
nodeid_t node = cpu_to_node(cpu);
struct desc_struct *gdt;
+unsigned int socket = cpu_to_socket(cpu);
+
if ( node != NUMA_NO_NODE )
Stray blank line being added.
@@ -717,6 +734,12 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
stack_base[0] = stack_start;
+socket_cpumask = xzalloc_array(cpumask_var_t, nr_sockets);
+if ( !socket_cpumask )
+panic(No memory for socket CPU siblings map);
+if ( !zalloc_cpumask_var(socket_cpumask) )
+panic(No memory for socket CPU siblings cpumask);
Please combine the two if()s to have just a single panic() invocation.
If either fails, it doesn't really matter which one it was.
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -58,6 +58,15 @@ int hard_smp_processor_id(void);
void __stop_this_cpu(void);
+/*
+ * The value may be greater than the actual socket number in the system and
+ * is considered not to change from the initial startup.
+ */
+extern unsigned int nr_sockets;
In the comment, instead of considered do you perhaps mean
expected or even required?
+/* Representing HT and core siblings in each socket */
+extern cpumask_var_t *socket_cpumask;
Comment style.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
