On Fri, Mar 27, 2015 at 08:16:10PM +0000, Andrew Cooper wrote:
> On 26/03/15 12:38, Chao Peng wrote:
> > @@ -238,6 +450,8 @@ static void cat_cpu_init(unsigned int cpu)
> > ASSERT(socket < nr_sockets);
> >
> > info = cat_socket_info + socket;
> > + if ( info->socket_cpu_mask == NULL )
> > + info->socket_cpu_mask = per_cpu(cpu_core_mask, cpu);
>
> Surely this wants to be skipped if info is already initialised?
The mask will be set to NULL in psr_cpu_fini() when the last CPU of the
socket is hot un-plugged. If any CPU of the socket is plugged in again
then the mask needs to be initialized but not skip.
>
> >
> > /* Avoid initializing more than one times for the same socket. */
> > if ( test_and_set_bool(info->initialized) )
> > @@ -254,6 +468,14 @@ static void cat_cpu_init(unsigned int cpu)
> > info->cbm_len = (eax & 0x1f) + 1;
> > info->cos_max = (edx & 0xffff);
> >
> > + info->cos_cbm_map = xzalloc_array(struct psr_cat_cbm,
> > + info->cos_max + 1UL);
> > + if ( !info->cos_cbm_map )
> > + return;
>
> This indicates that cat_cpu_init() needs to be able to signal ENOMEM.
The current behavior for ENOMEM case is: CAT is not enabled on the
socket. Both cat_cpu_init() and the caller will not actually make use
of this error code.
>
> > +
> > + /* cos=0 is reserved as default cbm(all ones). */
> > + info->cos_cbm_map[0].cbm = (1ull << info->cbm_len) - 1;
> > +
> > info->enabled = 1;
> > printk(XENLOG_DEBUG "CAT: enabled on socket %u, cos_max:%u,
> > cbm_len:%u\n",
> > socket, info->cos_max, info->cbm_len);
> > @@ -274,6 +496,24 @@ static void psr_cpu_init(unsigned int cpu)
> > psr_assoc_init();
> > }
> >
> > +static void psr_cpu_fini(unsigned int cpu)
> > +{
> > + unsigned int socket, next;
> > + cpumask_t *cpu_mask;
> > +
> > + if ( cat_socket_info )
> > + {
> > + socket = cpu_to_socket(cpu);
> > + cpu_mask = cat_socket_info[socket].socket_cpu_mask;
> > +
> > + if ( (next = cpumask_cycle(cpu, cpu_mask)) == cpu )
> > + cat_socket_info[socket].socket_cpu_mask = NULL;
> > + else
> > + cat_socket_info[socket].socket_cpu_mask =
> > + per_cpu(cpu_core_mask, next);
> > + }
> > +}
> > +
> Overall, this patch has a lot of moving parts in it. I have not spotted
> any major problems, but I also don't feel confident that I understand
> all of what is going on.
>
> It would certainly be easier to review if you split the patch into at
> least 3; the core infrastructure, the domctl and the sysctl bits. Even
> then, there appear to be several different bits of core changes going
> on, with some per-socket infrastructure and per-domain infrastructure.
OK, I will split it.
>
> The code itself appears to attempt to deal with sockets having a
> different quantity of COS entries, but how does it resolve having a
> different number of entries in the COS bitmaps? This would appear to
> mean that a domain given a certain COS would exhibit different behaviour
> depending on which socket it happened to be scheduled on.
Hypervisor maintains per-socket COS bitmask for each domain as well. So
a domain actually has COS bitmask for each socket but not only one COS
bitmask. xen_domctl_psr_cat_op interface allows toolstack to set/get
each COS bitmask on socket basis.
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
