| 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
| ||
|
Message-ID: <a2af90f4-5bce-df8d-2466-8dabe85dd4b7@redhat.com>
Date: Thu, 12 Aug 2021 09:34:00 +0200
From: David Hildenbrand <david@...hat.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Michael S. Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Dan Williams <dan.j.williams@...el.com>,
Hanjun Guo <guohanjun@...wei.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
"virtualization@...ts.linux-foundation.org"
<virtualization@...ts.linux-foundation.org>,
"linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize
iomem_is_exclusive()
On 12.08.21 09:14, Andy Shevchenko wrote:
>
>
> On Thursday, August 12, 2021, David Hildenbrand <david@...hat.com
> <mailto:david@...hat.com>> wrote:
>
> On 11.08.21 22:47, Andy Shevchenko wrote:
>
>
>
> On Wednesday, August 11, 2021, David Hildenbrand
> <david@...hat.com <mailto:david@...hat.com>
> <mailto:david@...hat.com <mailto:david@...hat.com>>> wrote:
>
> Let's clean it up a bit, removing the unnecessary usage of
> r_next() by
> next_resource(), and use next_range_resource() in case we
> are not
> interested in a certain subtree.
>
> Signed-off-by: David Hildenbrand <david@...hat.com
> <mailto:david@...hat.com>
> <mailto:david@...hat.com <mailto:david@...hat.com>>>
> ---
> kernel/resource.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 2938cf520ca3..ea853a075a83 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1754,9 +1754,8 @@ static int strict_iomem_checks;
> */
> bool iomem_is_exclusive(u64 addr)
> {
> - struct resource *p = &iomem_resource;
> + struct resource *p;
> bool err = false;
> - loff_t l;
> int size = PAGE_SIZE;
>
> if (!strict_iomem_checks)
> @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
> addr = addr & PAGE_MASK;
>
> read_lock(&resource_lock);
> - for (p = p->child; p ; p = r_next(NULL, p, &l)) {
> + for (p = iomem_resource.child; p ;) {
>
>
> Hi Andy,
>
>
> I consider the ordinal part of p initialization is slightly
> better and done outside of read lock.
>
> Something like
> p= &iomem_res...;
> read lock
> for (p = p->child; ...) {
>
>
> Why should we care about doing that outside of the lock? That smells
> like a micro-optimization the compiler will most probably overwrite
> either way as the address of iomem_resource is just constant?
>
> Also, for me it's much more readable and compact if we perform a
> single initialization instead of two separate ones in this case.
>
> We're using the pattern I use in, find_next_iomem_res() and
> __region_intersects(), while we use the old pattern in
> iomem_map_sanity_check(), where we also use the same unnecessary
> r_next() call.
>
> I might just cleanup iomem_map_sanity_check() in a similar way.
>
>
>
> Yes, it’s like micro optimization. If you want your way I suggest then
> to add a macro
>
> #define for_each_iomem_resource_child() \
> for (iomem_resource...)
I think the only thing that really makes sense would be something like this on top (not compiled yet):
diff --git a/kernel/resource.c b/kernel/resource.c
index ea853a075a83..35aaa72df0ce 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -80,6 +80,11 @@ static struct resource *next_resource_skip_children(struct resource *p)
return p->sibling;
}
+#define for_each_resource(_root, _p, _skip_children) \
+ for ((_p) = (_root)->child; (_p); \
+ (_p) = (_skip_children) ? next_resource_skip_children(_p) : \
+ next_resource(_p))
+
static void *r_next(struct seq_file *m, void *v, loff_t *pos)
{
struct resource *p = v;
@@ -1714,16 +1719,16 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
bool iomem_range_contains_excluded(u64 addr, u64 size)
{
const unsigned int flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_EXCLUSIVE;
- bool excluded = false;
+ bool skip_children, excluded = false;
struct resource *p;
read_lock(&resource_lock);
- for (p = iomem_resource.child; p ;) {
+ for_each_resource(&iomem_resource, p, skip_children) {
if (p->start >= addr + size)
break;
if (p->end < addr) {
/* No need to consider children */
- p = next_resource_skip_children(p);
+ skip_children = true;
continue;
}
/*
@@ -1735,7 +1740,7 @@ bool iomem_range_contains_excluded(u64 addr, u64 size)
excluded = true;
break;
}
- p = next_resource(p);
+ skip_children = false;
}
read_unlock(&resource_lock);
@@ -1755,7 +1760,7 @@ static int strict_iomem_checks;
bool iomem_is_exclusive(u64 addr)
{
struct resource *p;
- bool err = false;
+ bool skip_children, err = false;
int size = PAGE_SIZE;
if (!strict_iomem_checks)
@@ -1764,7 +1769,7 @@ bool iomem_is_exclusive(u64 addr)
addr = addr & PAGE_MASK;
read_lock(&resource_lock);
- for (p = iomem_resource.child; p ;) {
+ for_each_resource(&iomem_resource, p, skip_children) {
/*
* We can probably skip the resources without
* IORESOURCE_IO attribute?
@@ -1773,7 +1778,7 @@ bool iomem_is_exclusive(u64 addr)
break;
if (p->end < addr) {
/* No need to consider children */
- p = next_resource_skip_children(p);
+ skip_children = true;
continue;
}
@@ -1788,7 +1793,7 @@ bool iomem_is_exclusive(u64 addr)
err = true;
break;
}
- p = next_resource(p);
+ skip_children = false;
}
read_unlock(&resource_lock);
Thoughts?
--
Thanks,
David / dhildenb
Powered by blists - more mailing lists