On 12.08.21 09:14, Andy Shevchenko wrote:
On Thursday, August 12, 2021, David Hildenbrand <david@xxxxxxxxxx
<mailto:david@xxxxxxxxxx>> wrote:
On 11.08.21 22:47, Andy Shevchenko wrote:
On Wednesday, August 11, 2021, David Hildenbrand
<david@xxxxxxxxxx <mailto:david@xxxxxxxxxx>
<mailto:david@xxxxxxxxxx <mailto:david@xxxxxxxxxx>>> 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@xxxxxxxxxx
<mailto:david@xxxxxxxxxx>
<mailto:david@xxxxxxxxxx <mailto:david@xxxxxxxxxx>>>
---
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
