lkml.org 
[lkml]   [2012]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 3/3] Xen physical cpus interface
Date
Konrad,

Thanks for help me review!
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.

>
>
>> + switch (buf[0]) {
>
> Use strict_strtoull pls.

kernel suggest:
WARNING: strict_strtoull is obsolete, use kstrtoull instead :)

>
>> + case '0':
>> + ret = xen_pcpu_down(cpu->xen_id);
>> + break;
>> + case '1':
>> + ret = xen_pcpu_up(cpu->xen_id);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> +
>> + if (ret >= 0)
>> + ret = count;
>> + return ret;
>> +}
>> +static DEVICE_ATTR(online, S_IRUGO | S_IWUSR, show_online,
>> store_online);
>
>> +
>> +static ssize_t show_apicid(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct pcpu *cpu = container_of(dev, struct pcpu, dev); +
>> + return sprintf(buf, "%u\n", cpu->apic_id);
>> +}
>> +
>> +static ssize_t show_acpiid(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct pcpu *cpu = container_of(dev, struct pcpu, dev); +
>> + return sprintf(buf, "%u\n", cpu->acpi_id);
>> +}
>> +static DEVICE_ATTR(apic_id, S_IRUGO, show_apicid, NULL);
>> +static DEVICE_ATTR(acpi_id, S_IRUGO, show_acpiid, NULL);
>
> What benefit is there in exposing these values to the user?

Yes, no benefit so let's cancel exposing acpi_id and apic_id.

>> +
>> +static void pcpu_sys_remove(struct pcpu *pcpu)
>> +{
>> + struct device *dev;
>> +
>> + if (!pcpu)
>> + return;
>> +
>> + dev = &pcpu->dev;
>> + if (dev->id)
>> + device_remove_file(dev, &dev_attr_online);
>> + device_remove_file(dev, &dev_attr_acpi_id);
>> + device_remove_file(dev, &dev_attr_apic_id);
>> + device_unregister(dev);
>
> So .. if you are using that, can't you use the .release
> and define something like:
>
> static void pcpu_release(struct device *dev)
> {
> struct pcpu *pcpu = container_of(dev, struct pcpu, dev);
>
> list_del(&pcpu->pcpu_list);
> kfree(pcpu);
> }
>> +}
>> +
>> +static void free_pcpu(struct pcpu *pcpu)
>> +{
>> + if (!pcpu)
>> + return;
>> +
>> + pcpu_sys_remove(pcpu);
>
>> + list_del(&pcpu->pcpu_list);
>> + kfree(pcpu);
>
> Those two shouldn't be there. They sould be part of the
> release function.
>> +}
>> +
>> +static int pcpu_sys_create(struct pcpu *pcpu)
>> +{
>> + struct device *dev;
>> + int err = -EINVAL;
>> +
>> + if (!pcpu)
>> + return err;
>> +
>> + dev = &pcpu->dev;
>> + dev->bus = &xen_pcpu_subsys;
>> + dev->id = pcpu->xen_id;
>
> 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'.

>
>> + /* 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.

>> +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.

Thanks,
Jinsong--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-05-10 17:41    [W:0.063 / U:1.528 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site