| Message ID | 20210811203612.138506-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-mem: disallow mapping virtio-mem memory via /dev/mem | expand |
On Wednesday, August 11, 2021, David Hildenbrand <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> > --- > 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 ;) { 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; ...) { > /* > * We can probably skip the resources without > * IORESOURCE_IO attribute? > */ > if (p->start >= addr + size) > break; > - if (p->end < addr) > + if (p->end < addr) { > + /* No need to consider children */ > + p = next_resource_skip_children(p); > continue; > + } > + > /* > * A resource is exclusive if IORESOURCE_EXCLUSIVE is set > * or CONFIG_IO_STRICT_DEVMEM is enabled and the > * resource is busy. > */ > - if ((p->flags & IORESOURCE_BUSY) == 0) > - continue; > - if (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM) > - || p->flags & IORESOURCE_EXCLUSIVE) { > + if (p->flags & IORESOURCE_BUSY && > + (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM) || > + p->flags & IORESOURCE_EXCLUSIVE)) { > err = true; > break; > } > + p = next_resource(p); > } > read_unlock(&resource_lock); > > -- > 2.31.1 > >
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.
On Thursday, August 12, 2021, David Hildenbrand <david@redhat.com> wrote: > 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. > > 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 > >
On 12.08.21 09:14, Andy Shevchenko wrote: > > > On Thursday, August 12, 2021, David Hildenbrand <david@redhat.com > <mailto:david@redhat.com>> wrote: > > On 11.08.21 22:47, Andy Shevchenko wrote: > > > > On Wednesday, August 11, 2021, David Hildenbrand > <david@redhat.com <mailto:david@redhat.com> > <mailto: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> > <mailto: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. > > > > 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?
On Thu, Aug 12, 2021 at 09:34:00AM +0200, David Hildenbrand wrote: > On 12.08.21 09:14, Andy Shevchenko wrote: > > On Thursday, August 12, 2021, David Hildenbrand <david@redhat.com > > <mailto:david@redhat.com>> wrote: > > On 11.08.21 22:47, Andy Shevchenko wrote: > > On Wednesday, August 11, 2021, David Hildenbrand > > <david@redhat.com <mailto:david@redhat.com> > > <mailto:david@redhat.com <mailto:david@redhat.com>>> wrote: > > 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): Makes sense to me, thanks, go for it!
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 ;) { /* * We can probably skip the resources without * IORESOURCE_IO attribute? */ if (p->start >= addr + size) break; - if (p->end < addr) + if (p->end < addr) { + /* No need to consider children */ + p = next_resource_skip_children(p); continue; + } + /* * A resource is exclusive if IORESOURCE_EXCLUSIVE is set * or CONFIG_IO_STRICT_DEVMEM is enabled and the * resource is busy. */ - if ((p->flags & IORESOURCE_BUSY) == 0) - continue; - if (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM) - || p->flags & IORESOURCE_EXCLUSIVE) { + if (p->flags & IORESOURCE_BUSY && + (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM) || + p->flags & IORESOURCE_EXCLUSIVE)) { err = true; break; } + p = next_resource(p); } read_unlock(&resource_lock);
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> --- kernel/resource.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)