Re: [PATCH] Optimize the resource overlap check

[Richard Yang <weiyang@linux.vnet.ibm.com>]

On Mon, Apr 23, 2012 at 11:29:38AM -0600, Bjorn Helgaas wrote:
>On Mon, Apr 16, 2012 at 3:26 AM, Richard Yang
><weiyang@linux.vnet.ibm.com> wrote:
>>
>> In coalesce_windows() it tries to check whether the res1 and res2 overlap.
>> This function do four comparisons, which could be done with one
>> comparisons.
>>
>> Also make the resource_check_overlap() a common function for others.
>>
>> Signed-Off-By: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  arch/x86/pci/acpi.c    |   12 +-----------
>>  include/linux/ioport.h |    7 +++++++
>>  2 files changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index 68c3c13..f2bb99e 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -181,13 +181,6 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
>>        return AE_OK;
>>  }
>>
>> -static bool resource_contains(struct resource *res, resource_size_t point)
>> -{
>> -       if (res->start <= point && point <= res->end)
>> -               return true;
>> -       return false;
>> -}
>> -
>>  static void coalesce_windows(struct pci_root_info *info, unsigned long type)
>>  {
>>        int i, j;
>> @@ -208,10 +201,7 @@ static void coalesce_windows(struct pci_root_info *info, unsigned long type)
>>                         * our resources no longer match the ACPI _CRS, but
>>                         * the kernel resource tree doesn't allow overlaps.
>>                         */
>> -                       if (resource_contains(res1, res2->start) ||
>> -                           resource_contains(res1, res2->end) ||
>> -                           resource_contains(res2, res1->start) ||
>> -                           resource_contains(res2, res1->end)) {
>> +                       if (resource_check_overlap(res1, res2)) {
>>                                res1->start = min(res1->start, res2->start);
>>                                res1->end = max(res1->end, res2->end);
>>                                dev_info(&info->bridge->dev,
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index e9bb22c..374259b 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -198,5 +198,12 @@ extern int
>>  walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
>>                void *arg, int (*func)(unsigned long, unsigned long, void *));
>>
>> +static inline int resource_check_overlap(struct resource *r1,
>> +                                         struct resource *r2)
>> +{
>> +       return (r1->start <= r2->end && r1->end >= r2->start);
>> +}
>
>I like the concept, but resource_check_overlap() is not a very good
>name.  It returns a boolean, but the function name doesn't give any
>clue about the sense.  "resource_overlaps()" or something similar
>would be better.
change to this will be better?

static inline bool resource_overlaps(struct resource *r1,
                                         
>
>Bjorn

-- 
Richard Yang
Help you, Help me