From: David Hildenbrand <david@redhat.com> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>, Andrew Morton <akpm@linux-foundation.org>, Dan Williams <dan.j.williams@intel.com>, Hanjun Guo <guohanjun@huawei.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, "linux-mm@kvack.org" <linux-mm@kvack.org> Subject: Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive() Date: Thu, 12 Aug 2021 09:07:41 +0200 [thread overview] Message-ID: <37179df3-13d7-9b98-4cd8-13bb7f735129@redhat.com> (raw) In-Reply-To: <CAHp75VdQ_FkvBH4rw63mzm-4MymCWD2Ke_7Rf8T3Zmef3FeQVQ@mail.gmail.com> On 11.08.21 22:47, Andy Shevchenko wrote: > > > On Wednesday, August 11, 2021, David Hildenbrand <david@redhat.com > <mailto:david@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@redhat.com > <mailto:david@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
WARNING: multiple messages have this Message-ID (diff)
From: David Hildenbrand <david@redhat.com> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Arnd Bergmann <arnd@arndb.de>, "Michael S. Tsirkin" <mst@redhat.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, "linux-mm@kvack.org" <linux-mm@kvack.org>, Hanjun Guo <guohanjun@huawei.com>, Andrew Morton <akpm@linux-foundation.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Dan Williams <dan.j.williams@intel.com> Subject: Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive() Date: Thu, 12 Aug 2021 09:07:41 +0200 [thread overview] Message-ID: <37179df3-13d7-9b98-4cd8-13bb7f735129@redhat.com> (raw) In-Reply-To: <CAHp75VdQ_FkvBH4rw63mzm-4MymCWD2Ke_7Rf8T3Zmef3FeQVQ@mail.gmail.com> On 11.08.21 22:47, Andy Shevchenko wrote: > > > On Wednesday, August 11, 2021, David Hildenbrand <david@redhat.com > <mailto:david@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@redhat.com > <mailto:david@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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-08-12 7:07 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-11 20:36 [PATCH v1 0/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem David Hildenbrand 2021-08-11 20:36 ` David Hildenbrand 2021-08-11 20:36 ` [PATCH v1 1/3] /dev/mem: disallow access to explicitly excluded system RAM regions David Hildenbrand 2021-08-11 20:36 ` David Hildenbrand 2021-08-11 20:50 ` Andy Shevchenko 2021-08-11 20:50 ` Andy Shevchenko 2021-08-11 20:36 ` [PATCH v1 2/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem David Hildenbrand 2021-08-11 20:36 ` David Hildenbrand 2021-08-11 20:36 ` [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive() David Hildenbrand 2021-08-11 20:36 ` David Hildenbrand 2021-08-11 20:47 ` Andy Shevchenko 2021-08-11 20:47 ` Andy Shevchenko 2021-08-12 7:07 ` David Hildenbrand [this message] 2021-08-12 7:07 ` David Hildenbrand 2021-08-12 7:07 ` David Hildenbrand 2021-08-12 7:14 ` Andy Shevchenko 2021-08-12 7:14 ` Andy Shevchenko 2021-08-12 7:34 ` David Hildenbrand 2021-08-12 7:34 ` David Hildenbrand 2021-08-12 7:34 ` David Hildenbrand 2021-08-12 11:15 ` Andy Shevchenko 2021-08-12 11:15 ` Andy Shevchenko 2021-08-12 11:15 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=37179df3-13d7-9b98-4cd8-13bb7f735129@redhat.com \ --to=david@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=andy.shevchenko@gmail.com \ --cc=arnd@arndb.de \ --cc=dan.j.williams@intel.com \ --cc=gregkh@linuxfoundation.org \ --cc=guohanjun@huawei.com \ --cc=jasowang@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mst@redhat.com \ --cc=rafael.j.wysocki@intel.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.