Re: [Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
On Mon, Jun 01, 2015 at 09:36:15AM +0100, Jan Beulich wrote: > >>> On 01.06.15 at 10:05, wrote: > > 2) Unfeasible to allocate memory first and do initialization later in > > cpu hotplug notifications. My former approach is performing both the > > allocation and initialization in the CPU_STARTING, which is not a good > > idea indicated by Jan. > > Considering > > info->cos_max = min(opt_cos_max, edx & 0x); > > I don't see why you couldn't do an allocation using opt_cos_max > in CPU_UP_PREPARE, potentially not using all of the slots later > on. If memory waste is not a problem (especially in most cases opt_cos_max is its default 255), then this would be a solution. As you have suggested this, then I think I can go on this path and still have asymmetry systems been supported. Thanks, Chao ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
>>> On 01.06.15 at 10:05, wrote: > 2) Unfeasible to allocate memory first and do initialization later in > cpu hotplug notifications. My former approach is performing both the > allocation and initialization in the CPU_STARTING, which is not a good > idea indicated by Jan. Considering info->cos_max = min(opt_cos_max, edx & 0x); I don't see why you couldn't do an allocation using opt_cos_max in CPU_UP_PREPARE, potentially not using all of the slots later on. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
On Fri, May 29, 2015 at 04:38:31PM +0800, Chao Peng wrote:
> On Fri, May 29, 2015 at 09:06:46AM +0100, Jan Beulich wrote:
> > >>> On 29.05.15 at 04:43, wrote:
> > > On Thu, May 28, 2015 at 02:17:54PM +0100, Jan Beulich wrote:
> > >> >>> On 21.05.15 at 10:41, wrote:
> > >> > +static int cat_cpu_init(unsigned int cpu)
> > >> > +{
> > >> > +int rc;
> > >> > +const struct cpuinfo_x86 *c = cpu_data + cpu;
> > >> > +
> > >> > +if ( !cpu_has(c, X86_FEATURE_CAT) )
> > >> > +return 0;
> > >> > +
> > >> > +if ( test_bit(cpu_to_socket(cpu), cat_socket_enable) )
> > >> > +return 0;
> > >> > +
> > >> > +if ( cpu == smp_processor_id() )
> > >> > +do_cat_cpu_init(&rc);
> > >> > +else
> > >> > +on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, &rc, 1);
> > >>
> > >> This now being called in the context of CPU_UP_PREPARE, I can't see
> > >> how this works at all: Neither would the CPU's cpu_data[] instance be
> > >> initialized by that time, nor would you be able to IPI that CPU, nor can
> > >> I
> > >> see how the if() branch could ever get entered. Was this tested at all?
> > >
> > > Ah, yes! So it sounds really a little difficult to move the memory
> > > allocation from CPU_STARTING to CPU_PREPARA for this case.
> >
> > Not sure why you talk about memory allocation again. That should
> > be done in CPU_UP_PREPARE. But stuff that needs to happen on
> > the CPU should happen in CPU_STARTING. The memory allocation's
> > size depending on a CPU characteristic of course makes this a little
> > problematic, but (I think I said so before) since we're assuming
> > symmetry in many other places, I don't see anything wrong with
> > you assuming symmetry here too, and hence use e.g. the boot CPU's
> > value to determine the allocation size.
>
> No problem, then I can just forget the support for asymmetry in XEN.
As this is quite different with our original assumption and what I
described in the design doc, I'd like to have a clear summary here
before submitting the new version.
Basically speaking, the initial design tries to support systems have
different SKUs for each socket. One example would be when plugging one
HSX (Haswell server) and BDX (Broadwell server) processor into each
socket of a Grantley platform.
However, there are difficulties to support this than just to support
systems that always have the same SKUs, AFAICS:
1) Not able to detect nr_sockets correctly at booting time, especially
when taking cpu hotplug into account. This is also why I added a boot
option for this at the beginning of this patch serial, while I agreed
it's really not a good interface for user.
2) Unfeasible to allocate memory first and do initialization later in
cpu hotplug notifications. My former approach is performing both the
allocation and initialization in the CPU_STARTING, which is not a good
idea indicated by Jan.
For me, it's better to have it supported. But if that's difficult (just
as described above), I feel comfortable to drop it.
Let's see what changes will be made once that support is dropped:
1) Current per-socket psr_cat_socket_info will be dropped, instead
psr_cat will be introduced which holds cos_max/cbm_len for all the
sockets, and it will be initialized only once.
2) cat_socket_enable will be dropped as well, instead check
!!psr_cat, just as CMT did.
3) No special hotplug consideration, as the CAT hardware information on
booting cpu is applied to others.
4) We still support per socket cos configuration, so per socket
'struct psr_cat_cbm *cos_to_cbm' becomes something like
'struct psr_cat_cbm *socket_cbms' which holds all the cos_to_cbm for all
the sockets, and it will be initialized at booting time.
5) The 'target' in xen_sysctl_psr_cat_op will be removed as now all the
sockets have the same hardware info. Due to this, I'd like to retrofit
xen_sysctl_psr_cmt_op but not introduce a new sub-op for this.
XEN_DOMCTL_psr_cat_op however will still be there.
6) Related changes in tools side/documentation also should be done.
Though the list looks long but generally it becomes simple (of course).
Suggestions are welcome.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
On Fri, May 29, 2015 at 09:06:46AM +0100, Jan Beulich wrote:
> >>> On 29.05.15 at 04:43, wrote:
> > On Thu, May 28, 2015 at 02:17:54PM +0100, Jan Beulich wrote:
> >> >>> On 21.05.15 at 10:41, wrote:
> >> > +static int cat_cpu_init(unsigned int cpu)
> >> > +{
> >> > +int rc;
> >> > +const struct cpuinfo_x86 *c = cpu_data + cpu;
> >> > +
> >> > +if ( !cpu_has(c, X86_FEATURE_CAT) )
> >> > +return 0;
> >> > +
> >> > +if ( test_bit(cpu_to_socket(cpu), cat_socket_enable) )
> >> > +return 0;
> >> > +
> >> > +if ( cpu == smp_processor_id() )
> >> > +do_cat_cpu_init(&rc);
> >> > +else
> >> > +on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, &rc, 1);
> >>
> >> This now being called in the context of CPU_UP_PREPARE, I can't see
> >> how this works at all: Neither would the CPU's cpu_data[] instance be
> >> initialized by that time, nor would you be able to IPI that CPU, nor can I
> >> see how the if() branch could ever get entered. Was this tested at all?
> >
> > Ah, yes! So it sounds really a little difficult to move the memory
> > allocation from CPU_STARTING to CPU_PREPARA for this case.
>
> Not sure why you talk about memory allocation again. That should
> be done in CPU_UP_PREPARE. But stuff that needs to happen on
> the CPU should happen in CPU_STARTING. The memory allocation's
> size depending on a CPU characteristic of course makes this a little
> problematic, but (I think I said so before) since we're assuming
> symmetry in many other places, I don't see anything wrong with
> you assuming symmetry here too, and hence use e.g. the boot CPU's
> value to determine the allocation size.
No problem, then I can just forget the support for asymmetry in XEN.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
>>> On 29.05.15 at 04:43, wrote:
> On Thu, May 28, 2015 at 02:17:54PM +0100, Jan Beulich wrote:
>> >>> On 21.05.15 at 10:41, wrote:
>> > +static int cat_cpu_init(unsigned int cpu)
>> > +{
>> > +int rc;
>> > +const struct cpuinfo_x86 *c = cpu_data + cpu;
>> > +
>> > +if ( !cpu_has(c, X86_FEATURE_CAT) )
>> > +return 0;
>> > +
>> > +if ( test_bit(cpu_to_socket(cpu), cat_socket_enable) )
>> > +return 0;
>> > +
>> > +if ( cpu == smp_processor_id() )
>> > +do_cat_cpu_init(&rc);
>> > +else
>> > +on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, &rc, 1);
>>
>> This now being called in the context of CPU_UP_PREPARE, I can't see
>> how this works at all: Neither would the CPU's cpu_data[] instance be
>> initialized by that time, nor would you be able to IPI that CPU, nor can I
>> see how the if() branch could ever get entered. Was this tested at all?
>
> Ah, yes! So it sounds really a little difficult to move the memory
> allocation from CPU_STARTING to CPU_PREPARA for this case.
Not sure why you talk about memory allocation again. That should
be done in CPU_UP_PREPARE. But stuff that needs to happen on
the CPU should happen in CPU_STARTING. The memory allocation's
size depending on a CPU characteristic of course makes this a little
problematic, but (I think I said so before) since we're assuming
symmetry in many other places, I don't see anything wrong with
you assuming symmetry here too, and hence use e.g. the boot CPU's
value to determine the allocation size.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
On Thu, May 28, 2015 at 02:17:54PM +0100, Jan Beulich wrote:
> >>> On 21.05.15 at 10:41, wrote:
> > For each socket, a COS to CBM mapping structure is maintained for each
> > COS. The mapping is indexed by COS and the value is the corresponding
> > CBM. Different VMs may use the same CBM, a reference count is used to
> > indicate if the CBM is available.
> >
> > Signed-off-by: Chao Peng
> > Reviewed-by: Andrew Cooper
> > ---
> > Changes in v8:
> > * Move the memory allocation and CAT initialization code to CPU_UP_PREPARE.
> > * Add memory freeing code in CPU_DEAD path.
>
> Changes like this imo invalidate any tags given for earlier versions.
Sure, I will remove it.
> > +static int cat_cpu_init(unsigned int cpu)
> > +{
> > +int rc;
> > +const struct cpuinfo_x86 *c = cpu_data + cpu;
> > +
> > +if ( !cpu_has(c, X86_FEATURE_CAT) )
> > +return 0;
> > +
> > +if ( test_bit(cpu_to_socket(cpu), cat_socket_enable) )
> > +return 0;
> > +
> > +if ( cpu == smp_processor_id() )
> > +do_cat_cpu_init(&rc);
> > +else
> > +on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, &rc, 1);
>
> This now being called in the context of CPU_UP_PREPARE, I can't see
> how this works at all: Neither would the CPU's cpu_data[] instance be
> initialized by that time, nor would you be able to IPI that CPU, nor can I
> see how the if() branch could ever get entered. Was this tested at all?
Ah, yes! So it sounds really a little difficult to move the memory
allocation from CPU_STARTING to CPU_PREPARA for this case.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
>>> On 21.05.15 at 10:41, wrote:
> For each socket, a COS to CBM mapping structure is maintained for each
> COS. The mapping is indexed by COS and the value is the corresponding
> CBM. Different VMs may use the same CBM, a reference count is used to
> indicate if the CBM is available.
>
> Signed-off-by: Chao Peng
> Reviewed-by: Andrew Cooper
> ---
> Changes in v8:
> * Move the memory allocation and CAT initialization code to CPU_UP_PREPARE.
> * Add memory freeing code in CPU_DEAD path.
Changes like this imo invalidate any tags given for earlier versions.
> +
> +*rc = 0;
> +
> +return;
> +
> +}
> +
> +
Stray return and blank lines.
> +static int cat_cpu_init(unsigned int cpu)
> +{
> +int rc;
> +const struct cpuinfo_x86 *c = cpu_data + cpu;
> +
> +if ( !cpu_has(c, X86_FEATURE_CAT) )
> +return 0;
> +
> +if ( test_bit(cpu_to_socket(cpu), cat_socket_enable) )
> +return 0;
> +
> +if ( cpu == smp_processor_id() )
> +do_cat_cpu_init(&rc);
> +else
> +on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, &rc, 1);
This now being called in the context of CPU_UP_PREPARE, I can't see
how this works at all: Neither would the CPU's cpu_data[] instance be
initialized by that time, nor would you be able to IPI that CPU, nor can I
see how the if() branch could ever get entered. Was this tested at all?
> @@ -283,14 +331,24 @@ static void psr_cpu_fini(unsigned int cpu)
> static int cpu_callback(
> struct notifier_block *nfb, unsigned long action, void *hcpu)
> {
> +int rc = 0;
> unsigned int cpu = (unsigned long)hcpu;
>
> -if ( action == CPU_STARTING )
> -psr_cpu_init();
> -else if ( action == CPU_DEAD )
> +switch ( action )
> +{
> +case CPU_UP_PREPARE:
> +rc = psr_cpu_prepare(cpu);
> +break;
> +case CPU_STARTING:
> +psr_cpu_starting();
This not being run for the boot CPU, ...
> @@ -305,7 +363,7 @@ static int __init psr_presmp_init(void)
> if ( opt_psr & PSR_CAT )
> init_psr_cat();
>
> -psr_cpu_init();
> +psr_cpu_prepare(0);
> if ( psr_cmt_enabled() || cat_socket_info )
... don't you need to call it here too?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v8 03/13] x86: maintain COS to CBM mapping for each socket
For each socket, a COS to CBM mapping structure is maintained for each
COS. The mapping is indexed by COS and the value is the corresponding
CBM. Different VMs may use the same CBM, a reference count is used to
indicate if the CBM is available.
Signed-off-by: Chao Peng
Reviewed-by: Andrew Cooper
---
Changes in v8:
* Move the memory allocation and CAT initialization code to CPU_UP_PREPARE.
* Add memory freeing code in CPU_DEAD path.
Changes in v5:
* rename cos_cbm_map to cos_to_cbm.
---
xen/arch/x86/psr.c | 98 +++---
1 file changed, 78 insertions(+), 20 deletions(-)
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 471296e..8c844cb 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -21,9 +21,15 @@
#define PSR_CMT(1<<0)
#define PSR_CAT(1<<1)
+struct psr_cat_cbm {
+uint64_t cbm;
+unsigned int ref;
+};
+
struct psr_cat_socket_info {
unsigned int cbm_len;
unsigned int cos_max;
+struct psr_cat_cbm *cos_to_cbm;
};
struct psr_assoc {
@@ -208,34 +214,62 @@ void psr_ctxt_switch_to(struct domain *d)
psra->val = reg;
}
}
-
-static void cat_cpu_init(void)
+static void do_cat_cpu_init(void *arg)
{
unsigned int eax, ebx, ecx, edx;
-struct psr_cat_socket_info *info;
-unsigned int socket;
-unsigned int cpu = smp_processor_id();
-const struct cpuinfo_x86 *c = cpu_data + cpu;
-
-if ( !cpu_has(c, X86_FEATURE_CAT) )
-return;
-
-socket = cpu_to_socket(cpu);
-if ( test_bit(socket, cat_socket_enable) )
-return;
+int *rc = arg;
cpuid_count(PSR_CPUID_LEVEL_CAT, 0, &eax, &ebx, &ecx, &edx);
if ( ebx & PSR_RESOURCE_TYPE_L3 )
{
+struct psr_cat_socket_info *info;
+unsigned int socket = cpu_to_socket(smp_processor_id());
+
cpuid_count(PSR_CPUID_LEVEL_CAT, 1, &eax, &ebx, &ecx, &edx);
info = cat_socket_info + socket;
info->cbm_len = (eax & 0x1f) + 1;
info->cos_max = min(opt_cos_max, edx & 0x);
+info->cos_to_cbm = xzalloc_array(struct psr_cat_cbm,
+ info->cos_max + 1UL);
+if ( !info->cos_to_cbm )
+{
+*rc = -ENOMEM;
+return;
+}
+
+/* cos=0 is reserved as default cbm(all ones). */
+info->cos_to_cbm[0].cbm = (1ull << info->cbm_len) - 1;
+
set_bit(socket, cat_socket_enable);
printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u,
cbm_len:%u\n",
socket, info->cos_max, info->cbm_len);
}
+
+*rc = 0;
+
+return;
+
+}
+
+
+static int cat_cpu_init(unsigned int cpu)
+{
+int rc;
+const struct cpuinfo_x86 *c = cpu_data + cpu;
+
+if ( !cpu_has(c, X86_FEATURE_CAT) )
+return 0;
+
+if ( test_bit(cpu_to_socket(cpu), cat_socket_enable) )
+return 0;
+
+if ( cpu == smp_processor_id() )
+do_cat_cpu_init(&rc);
+else
+on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, &rc, 1);
+
+return rc;
}
static void cat_cpu_fini(unsigned int cpu)
@@ -243,7 +277,16 @@ static void cat_cpu_fini(unsigned int cpu)
unsigned int socket = cpu_to_socket(cpu);
if ( !socket_cpumask[socket] || cpumask_empty(socket_cpumask[socket]) )
+{
+struct psr_cat_socket_info *info = cat_socket_info + socket;
+
+if ( info->cos_to_cbm )
+{
+xfree(info->cos_to_cbm);
+info->cos_to_cbm = NULL;
+}
clear_bit(socket, cat_socket_enable);
+}
}
static void __init init_psr_cat(void)
@@ -266,11 +309,16 @@ static void __init init_psr_cat(void)
}
}
-static void psr_cpu_init(void)
+static int psr_cpu_prepare(unsigned int cpu)
{
if ( cat_socket_info )
-cat_cpu_init();
+return cat_cpu_init(cpu);
+return 0;
+}
+
+static void psr_cpu_starting(void)
+{
psr_assoc_init();
}
@@ -283,14 +331,24 @@ static void psr_cpu_fini(unsigned int cpu)
static int cpu_callback(
struct notifier_block *nfb, unsigned long action, void *hcpu)
{
+int rc = 0;
unsigned int cpu = (unsigned long)hcpu;
-if ( action == CPU_STARTING )
-psr_cpu_init();
-else if ( action == CPU_DEAD )
+switch ( action )
+{
+case CPU_UP_PREPARE:
+rc = psr_cpu_prepare(cpu);
+break;
+case CPU_STARTING:
+psr_cpu_starting();
+break;
+case CPU_UP_CANCELED:
+case CPU_DEAD:
psr_cpu_fini(cpu);
+break;
+}
-return NOTIFY_DONE;
+return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
}
static struct notifier_block cpu_nfb = {
@@ -305,7 +363,7 @@ static int __init psr_presmp_init(void)
if ( opt_psr & PSR_CAT )
init_psr_cat();
-psr_cpu_init();
+psr_cpu_prepare(0);
if ( psr_cmt_enabled() || cat_socket_info )
register_cpu_notifier(&cpu_nfb);
--
1.9.1
___
Xen-devel maili
