Re: [PATCH 3/3] Xen physical cpus interface

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, May 10, 2012 at 02:54:07PM +0000, Liu, Jinsong wrote:
> Konrad,
> 
> Thanks for help me review!

Sure thing.
> Update according to your suggestion. 
> Add some comments below.
> 
> >> 
> >> Manage physical cpus in dom0, get physical cpus info and provide sys
> >> interface. 
> > 
> > Anything that exposes SysFS attributes needs documentation in
> > Documentation/ABI
> 
> Yes, added.
> 
> > 
> > Can you explain what this solves? And if there are any
> > userland applications that use this?
> > 
> 
> It provide cpu online/offline interface to user. User can use it for their own purpose, like power saving - by offlining some cpus when light workload it save power greatly.

OK, please include that in the descritpion.

> 
> > 
> > 
> >> +	switch (buf[0]) {
> > 
> > Use strict_strtoull pls.
> 
> kernel suggest:
> WARNING: strict_strtoull is obsolete, use kstrtoull instead :)

Ah yes.
.. snip..
> > And then here dev->release = &pcpu_release;
> > 
> 
> Hmm, it's good if it's convenient to do it automatically via dev->release.
> However, dev container (pcpu) would be free at some other error cases, so I prefer do it 'manually'.

You could also call pcpu_release(..) to do it manually.

> 
> > 
> >> +	/* Not open pcpu0 online access to user */
> > 
> > Huh? You mean "Nobody can touch PCPU0" ?
> 
> Add comments:
>         /*
>          * Xen never offline cpu0 due to several restrictions
>          * and assumptions. This basically doesn't add a sys control
>          * to user, one cannot attempt to offline BSP.
>          */
> 
> > 
> > Why? Why can they touch the other ones? And better yet,
> > what happens if one boots without "dom0_max_vcpus=X"
> > and powers of some of the CPUs?
> > 
> 
> Only those at cpu present map has its sys interface.

OK, put that in the file so folks are aware of the limitations.

> 
> >> +static int __init xen_pcpu_init(void)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (!xen_initial_domain())
> >> +		return -ENODEV;
> >> +
> >> +	ret = subsys_system_register(&xen_pcpu_subsys, NULL); +	if (ret) {
> >> +		pr_warning(XEN_PCPU "Failed to register pcpu subsys\n");
> >> +		return ret; +	}
> >> +
> >> +	INIT_LIST_HEAD(&xen_pcpus.list);
> >> +
> >> +	ret = xen_sync_pcpus();
> >> +	if (ret) {
> >> +		pr_warning(XEN_PCPU "Failed to sync pcpu info\n"); +		return ret;
> >> +	}
> >> +
> >> +	ret = bind_virq_to_irqhandler(VIRQ_PCPU_STATE, 0,
> >> +				      xen_pcpu_interrupt, 0,
> >> +				      "pcpu", NULL);
> > 
> > "xen-pcpu"
> > 
> >> +	if (ret < 0) {
> >> +		pr_warning(XEN_PCPU "Failed to bind pcpu virq\n");
> > 
> > Shouldn't you delete what 'xen_sync_pcpus' did?
> 
> yes, add error handling.
> 
> > Or is it OK to still work without the interrupts? What is the purpose
> > of that interrupt? How does it actually work - the hypervisor
> > decides when/where to turn off CPUs?
> > 
> 
> user online/offline cpu via sys interface --> xen implement --> inject virq back to dom0 --> sync cpu status.

Add that in the file so the workflow is explained.
> 
> Thanks,
> Jinsong