linux-kernel - Re: [PATCH] Resource: fix region

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r0a2g64n.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Mon, 02 Sep 2024 10:07:52 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Dan Williams <dan.j.williams@...el.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,  linux-mm@...ck.org,
  linux-kernel@...r.kernel.org,  linux-cxl@...r.kernel.org,  Davidlohr
 Bueso <dave@...olabs.net>,  Jonathan Cameron
 <jonathan.cameron@...wei.com>,  Dave Jiang <dave.jiang@...el.com>,  Alison
 Schofield <alison.schofield@...el.com>,  Vishal Verma
 <vishal.l.verma@...el.com>,  Ira Weiny <ira.weiny@...el.com>,  Alistair
 Popple <apopple@...dia.com>,  Andy Shevchenko
 <andriy.shevchenko@...ux.intel.com>,  Bjorn Helgaas <bhelgaas@...gle.com>,
  Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH] Resource: fix region_intersects() for CXL memory

Dan Williams <dan.j.williams@...el.com> writes:

> Huang, Ying wrote:
>> Dan Williams <dan.j.williams@...el.com> writes:
>> > Huang Ying wrote:
> [..]
>> >>  kernel/resource.c | 44 +++++++++++++++++++++++++++++++++++++-------
>> >>  1 file changed, 37 insertions(+), 7 deletions(-)
>> >> 
>> >> diff --git a/kernel/resource.c b/kernel/resource.c
>> >> index 14777afb0a99..c97a5add9394 100644
>> >> --- a/kernel/resource.c
>> >> +++ b/kernel/resource.c
> [..]

[snip]

>
>> However, I admit that the original code is hard to be understood,
>> whether is something like below better?
>
> I like that this proposal defers incrementing @other rather than
> decrement after the fact.
>
>> 
>> 	for (p = parent->child; p ; p = p->sibling) {
>> 		if (!resource_overlaps(p, &res))
>> 			continue;
>> 		is_type = (((p->flags & flags) == flags) &&
>> 			   ((desc == IORES_DESC_NONE) || (desc == p->desc)));
>> 		if (is_type) {
>> 			type++;
>> 			continue;
>> 		}
>> 		/*
>> 		 * Continue to search in descendant resources.  Unless
>> 		 * the matched descendant resources cover the whole
>> 		 * overlapped range, increase 'other', because it
>> 		 * overlaps with 'p' at least.
>> 		 */
>> 		covered = false;
>
> I would call @covered, @single_descendant. Since @covered is ambiguous.

Sorry, I don't understand why this is called @single_descendant.  It's
possible that the checked region is overlapped with 2 descendants, and
the result is REGION_INTERSECTS.  For example,

490000000-52fffffff : CXL Window 0
  490000000-50fffffff : region0
    490000000-50fffffff : dax0.0
      490000000-50fffffff : System RAM (kmem)
  510000000-52fffffff : region1
    510000000-52fffffff : dax0.1
      510000000-52fffffff : System RAM (kmem)

region_intersects(, 0x50ffff000, 2 * PAGE_SIZE, IORESOURCE_SYSTEM_RAM,
IORES_DESC_NONE) => REGION_INTERSECTS

>> 		ostart = max(res.start, p->start);
>> 		oend = min(res.end, p->end);
>> 		for_each_resource(p, dp, false) {
>> 			if (!resource_overlaps(dp, &res))
>> 				continue;
>> 			is_type = (((dp->flags & flags) == flags) &&
>> 				   ((desc == IORES_DESC_NONE) ||
>> 				    (desc == dp->desc)));
>> 			if (is_type) {
>> 				type++;
>> 				if (dp->start > ostart)
>
> ...this should have a comment:
>
> /* partial descendant overlap indicates overlap with a descendant hole */

Yes.  Some comments should help.

>> 					break;
>> 				if (dp->end >= oend) {
>> 					covered = true;
>> 					break;
>
> ...then per above this because easier to read as:
>
> 	single_descendant = true;
>
>> 				}
>> 				ostart = dp->end + 1;
>> 			}
>> 		}
>> 		if (!covered)
>> 			other++;
>> 	}
>> 

[snip]

--
Best Regards,
Huang, Ying