memblock glitch

memblock glitch

[Benjamin Herrenschmidt]

Hi Russell !

While looking at updating ARM memblock to some of my changes, I found
this bit in arch/arm/mm/init.c:

int pfn_valid(unsigned long pfn)
{
	struct memblock_region *mem = &memblock.memory;
	unsigned int left = 0, right = mem->cnt;

	do {
		unsigned int mid = (right + left) / 2;

		if (pfn < memblock_start_pfn(mem, mid))
			right = mid;
		else if (pfn >= memblock_end_pfn(mem, mid))
			left = mid + 1;
		else
			return 1;
	} while (left < right);
	return 0;
}
EXPORT_SYMBOL(pfn_valid);

So you're trying to do a dichotomy to be faster. However, that doesn't
quite work with my new code as I'm trying to take away access to
the internals of memblock from arch code so we can re-implement the core
in a slightly saner way if we wish to.

Since I understand why you don't want a linear search there, any
objection if I move the logic to the core memblock and expose it via a
memblock_search() kind of facility ?

Cheers,
Ben.

memblock glitch

[Benjamin Herrenschmidt]

On Wed, 2010-08-04 at 10:38 +1000, Benjamin Herrenschmidt wrote:
> Hi Russell !
> 
> While looking at updating ARM memblock to some of my changes, I found
> this bit in arch/arm/mm/init.c:

And another question...

In arch/arm/plat-omap/fb.c:

static int valid_sdram(unsigned long addr, unsigned long size)
{
	struct memblock_region res;

	res.base = addr;
	res.size = size;
	return !memblock_find(&res) && res.base == addr && res.size == size;
}

So if I understand things correctly, you are working around the weird
behaviour of memblock_find(), which returns the intersection of the
region passed and the first memblock that partially overlaps it.

Since you are now the only user of that function (I was about to remove it),
would you be happy if I replaced the above and the !SPARSEMEM pfn_valid()
with a single function: memblock_is_mem(addr, size) ?

It can do a fast search (binary search or whatever) on addr, and then
dbl check size (which would be PAGE_SIZE for pfn_valid).

Ben.

memblock_is_region_reserved() issue

[Benjamin Herrenschmidt]

Hi folks !

You seem to use memblock_is_region_reserved() in a few places in the new
code assuming (as I would) that it returns a boolean (0 reserved, !0 not
reserved).

However, the implementation looks totally wrong. I was wondering if you
had noticed :-)

I'm about to send a fix, but wanted to make sure you guys were ok with
it.

Cheers,
Ben.

memblock glitch

[Russell King - ARM Linux]

On Wed, Aug 04, 2010 at 10:38:27AM +1000, Benjamin Herrenschmidt wrote:
> Hi Russell !
> 
> While looking at updating ARM memblock to some of my changes, I found
> this bit in arch/arm/mm/init.c:
> 
> int pfn_valid(unsigned long pfn)
> {
> 	struct memblock_region *mem = &memblock.memory;
> 	unsigned int left = 0, right = mem->cnt;
> 
> 	do {
> 		unsigned int mid = (right + left) / 2;
> 
> 		if (pfn < memblock_start_pfn(mem, mid))
> 			right = mid;
> 		else if (pfn >= memblock_end_pfn(mem, mid))
> 			left = mid + 1;
> 		else
> 			return 1;
> 	} while (left < right);
> 	return 0;
> }
> EXPORT_SYMBOL(pfn_valid);
> 
> So you're trying to do a dichotomy to be faster.

Correct.  The normal size of this thing tends to typically be one or
maybe two entries, although we normally support up to 8 ranges, or 16
in one case.  It works well because of the tight cache locality of the
data, which occupies maybe up to three cache lines.

> However, that doesn't
> quite work with my new code as I'm trying to take away access to
> the internals of memblock from arch code so we can re-implement the core
> in a slightly saner way if we wish to.
> 
> Since I understand why you don't want a linear search there, any
> objection if I move the logic to the core memblock and expose it via a
> memblock_search() kind of facility ?

No objection.

memblock glitch

[Russell King - ARM Linux]

On Wed, Aug 04, 2010 at 11:38:10AM +1000, Benjamin Herrenschmidt wrote:
> In arch/arm/plat-omap/fb.c:
> 
> static int valid_sdram(unsigned long addr, unsigned long size)
> {
> 	struct memblock_region res;
> 
> 	res.base = addr;
> 	res.size = size;
> 	return !memblock_find(&res) && res.base == addr && res.size == size;
> }
> 
> So if I understand things correctly, you are working around the weird
> behaviour of memblock_find(), which returns the intersection of the
> region passed and the first memblock that partially overlaps it.

Correct - what it's trying to ascertain is whether the address range
is entirely contained within a valid region.

> Since you are now the only user of that function (I was about to remove it),
> would you be happy if I replaced the above and the !SPARSEMEM pfn_valid()
> with a single function: memblock_is_mem(addr, size) ?
> 
> It can do a fast search (binary search or whatever) on addr, and then
> dbl check size (which would be PAGE_SIZE for pfn_valid).

You could do, but do we want to introduce size checks for pfn_valid?
I'm slightly concerned because it can be a hot path.

If all entries in memblock are already page aligned, if addr falls
within a memblock, it must cover the entire page so checking the size
seems redundant for this case.

memblock glitch

[Benjamin Herrenschmidt]

On Wed, 2010-08-04 at 09:54 +0100, Russell King - ARM Linux wrote:
> 
> You could do, but do we want to introduce size checks for pfn_valid?
> I'm slightly concerned because it can be a hot path.
> 
> If all entries in memblock are already page aligned, if addr falls
> within a memblock, it must cover the entire page so checking the size
> seems redundant for this case. 

Ok, I've done a separate function for single address and range for now.
Feel free to look at my memblock branch in powerpc.git.

I'm still thinking however whether I should just expose a
memblock_pfn_valid() that can be directly hooked up to arch that wants
it to limit the overhead to a minimum..

Cheers,
Ben.

memblock_is_region_reserved() issue

[Russell King - ARM Linux]

On Wed, Aug 04, 2010 at 01:58:47PM +1000, Benjamin Herrenschmidt wrote:
> Hi folks !
> 
> You seem to use memblock_is_region_reserved() in a few places in the new
> code assuming (as I would) that it returns a boolean (0 reserved, !0 not
> reserved).

That's where having a comment describing such things would be useful,
so that it's easier to ensure that it's correct at creation/review time.

> However, the implementation looks totally wrong. I was wondering if you
> had noticed :-)

I guess not - I relied on others to test that code... no reports were
forthcoming of breakage.  Maybe the test cases didn't involve paths
through this code though.