David Hildenbrand
2021-Aug-12 07:07 UTC
[PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()
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. -- Thanks, David / dhildenb
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>