Andy Shevchenko
2021-Aug-12 07:14 UTC
[PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()
On Thursday, August 12, 2021, David Hildenbrand <david at redhat.com> wrote:> On 11.08.21 22:47, Andy Shevchenko wrote: > >> >> >> On Wednesday, August 11, 2021, David Hildenbrand <david at redhat.com >> <mailto:david at redhat.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 at redhat.com >> <mailto:david at redhat.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...)> > Thanks, > > David / dhildenb > >-- With Best Regards, Andy Shevchenko -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210812/7b418576/attachment.html>
David Hildenbrand
2021-Aug-12 07:34 UTC
[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 at redhat.com > <mailto:david at redhat.com>> wrote: > > On 11.08.21 22:47, Andy Shevchenko wrote: > > > > On Wednesday, August 11, 2021, David Hildenbrand > <david at redhat.com <mailto:david at redhat.com> > <mailto:david at redhat.com <mailto:david at redhat.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 at redhat.com > <mailto:david at redhat.com> > ? ? <mailto:david at redhat.com <mailto:david at redhat.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