linux-kernel - Re: [PATCH v1 2/5] kernel/resource: merge_system_ram_resources() to merge resources after hotplug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 8 Sep 2020 12:26:20 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Pankaj Gupta <pankaj.gupta.linux@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        virtualization@...ts.linux-foundation.org,
        Linux MM <linux-mm@...ck.org>, linux-hyperv@...r.kernel.org,
        xen-devel@...ts.xenproject.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Kees Cook <keescook@...omium.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "K. Y. Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Roger Pau Monné <roger.pau@...rix.com>,
        Julien Grall <julien@....org>, Baoquan He <bhe@...hat.com>,
        Wei Yang <richardw.yang@...ux.intel.com>
Subject: Re: [PATCH v1 2/5] kernel/resource: merge_system_ram_resources() to
 merge resources after hotplug

On 31.08.20 11:35, Pankaj Gupta wrote:
>> Some add_memory*() users add memory in small, contiguous memory blocks.
>> Examples include virtio-mem, hyper-v balloon, and the XEN balloon.
>>
>> This can quickly result in a lot of memory resources, whereby the actual
>> resource boundaries are not of interest (e.g., it might be relevant for
>> DIMMs, exposed via /proc/iomem to user space). We really want to merge
>> added resources in this scenario where possible.
>>
>> Let's provide an interface to trigger merging of applicable child
>> resources. It will be, for example, used by virtio-mem to trigger
>> merging of system ram resources it added to its resource container, but
>> also by XEN and Hyper-V to trigger merging of system ram resources in
>> iomem_resource.
>>
>> Note: We really want to merge after the whole operation succeeded, not
>> directly when adding a resource to the resource tree (it would break
>> add_memory_resource() and require splitting resources again when the
>> operation failed - e.g., due to -ENOMEM).
>>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: Dan Williams <dan.j.williams@...el.com>
>> Cc: Jason Gunthorpe <jgg@...pe.ca>
>> Cc: Kees Cook <keescook@...omium.org>
>> Cc: Ard Biesheuvel <ardb@...nel.org>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: "K. Y. Srinivasan" <kys@...rosoft.com>
>> Cc: Haiyang Zhang <haiyangz@...rosoft.com>
>> Cc: Stephen Hemminger <sthemmin@...rosoft.com>
>> Cc: Wei Liu <wei.liu@...nel.org>
>> Cc: Boris Ostrovsky <boris.ostrovsky@...cle.com>
>> Cc: Juergen Gross <jgross@...e.com>
>> Cc: Stefano Stabellini <sstabellini@...nel.org>
>> Cc: Roger Pau Monné <roger.pau@...rix.com>
>> Cc: Julien Grall <julien@....org>
>> Cc: Pankaj Gupta <pankaj.gupta.linux@...il.com>
>> Cc: Baoquan He <bhe@...hat.com>
>> Cc: Wei Yang <richardw.yang@...ux.intel.com>
>> Signed-off-by: David Hildenbrand <david@...hat.com>
>> ---
>>  include/linux/ioport.h |  3 +++
>>  kernel/resource.c      | 52 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 55 insertions(+)
>>
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index 52a91f5fa1a36..3bb0020cd6ddc 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -251,6 +251,9 @@ extern void __release_region(struct resource *, resource_size_t,
>>  extern void release_mem_region_adjustable(struct resource *, resource_size_t,
>>                                           resource_size_t);
>>  #endif
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +extern void merge_system_ram_resources(struct resource *res);
>> +#endif
>>
>>  /* Wrappers for managed devices */
>>  struct device;
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 1dcef5d53d76e..b4e0963edadd2 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1360,6 +1360,58 @@ void release_mem_region_adjustable(struct resource *parent,
>>  }
>>  #endif /* CONFIG_MEMORY_HOTREMOVE */
>>
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +static bool system_ram_resources_mergeable(struct resource *r1,
>> +                                          struct resource *r2)
>> +{
>> +       return r1->flags == r2->flags && r1->end + 1 == r2->start &&
>> +              r1->name == r2->name && r1->desc == r2->desc &&
>> +              !r1->child && !r2->child;
>> +}
>> +
>> +/*
>> + * merge_system_ram_resources - try to merge contiguous system ram resources
>> + * @parent: parent resource descriptor
>> + *
>> + * This interface is intended for memory hotplug, whereby lots of contiguous
>> + * system ram resources are added (e.g., via add_memory*()) by a driver, and
>> + * the actual resource boundaries are not of interest (e.g., it might be
>> + * relevant for DIMMs). Only immediate child resources that are busy and
>> + * don't have any children are considered. All applicable child resources
>> + * must be immutable during the request.
>> + *
>> + * Note:
>> + * - The caller has to make sure that no pointers to resources that might
>> + *   get merged are held anymore. Callers should only trigger merging of child
>> + *   resources when they are the only one adding system ram resources to the
>> + *   parent (besides during boot).
>> + * - release_mem_region_adjustable() will split on demand on memory hotunplug
>> + */
>> +void merge_system_ram_resources(struct resource *parent)
>> +{
>> +       const unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>> +       struct resource *cur, *next;
>> +
>> +       write_lock(&resource_lock);
>> +
>> +       cur = parent->child;
>> +       while (cur && cur->sibling) {
>> +               next = cur->sibling;
>> +               if ((cur->flags & flags) == flags &&
> 
> Maybe this can be changed to:
> !(cur->flags & ~flags)

That would be different I think.

(cur->flags & flags) == flags
checks that all "flags" are set (additional ones might be set).

!(cur->flags & ~flags)
checks that no other flags besides "flags" are set (and "flags" are not
required to be set).


We use the same handling in find_next_iomem_res(), e.g., called via
walk_system_ram_range also with IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY.

Thanks for having a look!

-- 
Thanks,

David / dhildenb