ioremap()/iounmap() problem

ioremap()/iounmap() problem

[Tomi Valkeinen]

Hi,

Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
with iounmap(). It looks like iounmap() does not properly free large
areas. Below is a test which fails for me in 6-7 loops.

OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
generic ARM code. I looked at the ioremap code and for larger areas the
code uses area sections, and I believe the bug is somewhere there. 

Does this work on other processors?

 Tomi



#include <linux/module.h>
#include <linux/io.h>

static int test_init(void)
{
	const unsigned long paddr = 0x70000000;
	const unsigned long size = 2048 * 2048 * 4;
	void *vaddr;
	int i;

	for (i = 0; i < 200; ++i) {
		vaddr = ioremap(paddr, size);

		if (!vaddr) {
			printk("couldn't ioremap\n");
			break;
		}

		printk("ioremapped to %p\n", vaddr);

		iounmap(vaddr);
	}

	return 0;
}

static void test_exit(void) { }

module_init(test_init);
module_exit(test_exit);

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 12:23:03PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> with iounmap(). It looks like iounmap() does not properly free large
> areas. Below is a test which fails for me in 6-7 loops.
> 
> OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> generic ARM code. I looked at the ioremap code and for larger areas the
> code uses area sections, and I believe the bug is somewhere there. 

In unmap_area_sections(), try changing:

        unsigned long addr = virt, end = virt + (size & ~SZ_1M);

to

        unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));

Re: ioremap()/iounmap() problem

[Tomi Valkeinen]

Hi,

On Mon, 2009-01-19 at 11:01 +0000, ext Russell King - ARM Linux wrote:
> On Mon, Jan 19, 2009 at 12:23:03PM +0200, Tomi Valkeinen wrote:
> > Hi,
> > 
> > Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> > with iounmap(). It looks like iounmap() does not properly free large
> > areas. Below is a test which fails for me in 6-7 loops.
> > 
> > OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> > generic ARM code. I looked at the ioremap code and for larger areas the
> > code uses area sections, and I believe the bug is somewhere there. 
> 
> In unmap_area_sections(), try changing:
> 
>         unsigned long addr = virt, end = virt + (size & ~SZ_1M);
> 
> to
> 
>         unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));

This didn't help. I Added two debug prints, not sure if it means
anything but the size on the second unmap is not the same as in the
previous ones.

remap_area_sections(virt=d2000000, size=1000000, end=d3000000, pfn=70000)
unmap_area_sections(virt=d2000000, size=1000000, end=d3000000)
ioremapped to d2000000
unmap_area_sections(virt=d2000000, size=1001000, end=d3000000)

 Tomi

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 02:27:34PM +0200, Tomi Valkeinen wrote:
> On Mon, 2009-01-19 at 11:01 +0000, ext Russell King - ARM Linux wrote:
> > On Mon, Jan 19, 2009 at 12:23:03PM +0200, Tomi Valkeinen wrote:
> > > Hi,
> > > 
> > > Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> > > with iounmap(). It looks like iounmap() does not properly free large
> > > areas. Below is a test which fails for me in 6-7 loops.
> > > 
> > > OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> > > generic ARM code. I looked at the ioremap code and for larger areas the
> > > code uses area sections, and I believe the bug is somewhere there. 
> > 
> > In unmap_area_sections(), try changing:
> > 
> >         unsigned long addr = virt, end = virt + (size & ~SZ_1M);
> > 
> > to
> > 
> >         unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));
> 
> This didn't help. I Added two debug prints, not sure if it means
> anything but the size on the second unmap is not the same as in the
> previous ones.

If you read the comments on unmap_area_sections, you'd understand why
this is.

> remap_area_sections(virt=d2000000, size=1000000, end=d3000000, pfn=70000)
> unmap_area_sections(virt=d2000000, size=1000000, end=d3000000)
> ioremapped to d2000000
> unmap_area_sections(virt=d2000000, size=1001000, end=d3000000)

Yes, so changing that line results in 'end' being the correct 0x1000000
bytes greater than 'virt', as can be seen from the above.  Before this
change, end would've been 0xd3001000 which is incorrect.

Now, if there's something else wrong, please be more expansive about it.
Please given an exact description of the failure you're seeing.  (I'm
not going to be able to try your test program for several days.)

Re: ioremap()/iounmap() problem

[Tomi Valkeinen]

On Mon, 2009-01-19 at 12:53 +0000, ext Russell King - ARM Linux wrote:
> On Mon, Jan 19, 2009 at 02:27:34PM +0200, Tomi Valkeinen wrote:
> > On Mon, 2009-01-19 at 11:01 +0000, ext Russell King - ARM Linux wrote:
> > > On Mon, Jan 19, 2009 at 12:23:03PM +0200, Tomi Valkeinen wrote:
> > > > Hi,
> > > > 
> > > > Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> > > > with iounmap(). It looks like iounmap() does not properly free large
> > > > areas. Below is a test which fails for me in 6-7 loops.
> > > > 
> > > > OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> > > > generic ARM code. I looked at the ioremap code and for larger areas the
> > > > code uses area sections, and I believe the bug is somewhere there. 
> > > 
> > > In unmap_area_sections(), try changing:
> > > 
> > >         unsigned long addr = virt, end = virt + (size & ~SZ_1M);
> > > 
> > > to
> > > 
> > >         unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));
> > 
> > This didn't help. I Added two debug prints, not sure if it means
> > anything but the size on the second unmap is not the same as in the
> > previous ones.
> 
> If you read the comments on unmap_area_sections, you'd understand why
> this is.
> 
> > remap_area_sections(virt=d2000000, size=1000000, end=d3000000, pfn=70000)
> > unmap_area_sections(virt=d2000000, size=1000000, end=d3000000)
> > ioremapped to d2000000
> > unmap_area_sections(virt=d2000000, size=1001000, end=d3000000)
> 
> Yes, so changing that line results in 'end' being the correct 0x1000000
> bytes greater than 'virt', as can be seen from the above.  Before this
> change, end would've been 0xd3001000 which is incorrect.
> 
> Now, if there's something else wrong, please be more expansive about it.
> Please given an exact description of the failure you're seeing.  (I'm
> not going to be able to try your test program for several days.)

The original problem remains, it looks to me that iounmap() doesn't free
the memory, and thus ioremap will fail after couple of loops.

I get the following error on console:

vmap allocation for size 16781312 failed: use vmalloc=<size> to increase
size.

I don't know too much about memory management, but I'll try to dig
deeper in to this.

What I don't understand is that in __arm_ioremap_pfn() there's
get_vm_area(), and later in the function, in case of error, that area is
vunmap()ed. But there's no vunmap() done anywhere else in the code for
area sections, so who does free it?

 Tomi

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 03:49:40PM +0200, Tomi Valkeinen wrote:
> The original problem remains, it looks to me that iounmap() doesn't free
> the memory, and thus ioremap will fail after couple of loops.
> 
> I get the following error on console:
> 
> vmap allocation for size 16781312 failed: use vmalloc=<size> to increase
> size.
> 
> I don't know too much about memory management, but I'll try to dig
> deeper in to this.
> 
> What I don't understand is that in __arm_ioremap_pfn() there's
> get_vm_area(), and later in the function, in case of error, that area is
> vunmap()ed. But there's no vunmap() done anywhere else in the code for
> area sections, so who does free it?

Yes, the error I pointed out is not the only issue with this code.

You might consider trying my patch which was sent in this thread to Matt
Gerassimoff.  And if you see anyone whinging that it won't work for
whatever reason, please ignore them and just test the patch.  You will
not be disappointed.

Re: ioremap()/iounmap() problem

[Tomi Valkeinen]

Hi,

On Wed, 2009-01-21 at 19:23 +0000, ext Russell King - ARM Linux wrote:
> On Mon, Jan 19, 2009 at 03:49:40PM +0200, Tomi Valkeinen wrote:
> > The original problem remains, it looks to me that iounmap() doesn't free
> > the memory, and thus ioremap will fail after couple of loops.
> > 
> > I get the following error on console:
> > 
> > vmap allocation for size 16781312 failed: use vmalloc=<size> to increase
> > size.
> > 
> > I don't know too much about memory management, but I'll try to dig
> > deeper in to this.
> > 
> > What I don't understand is that in __arm_ioremap_pfn() there's
> > get_vm_area(), and later in the function, in case of error, that area is
> > vunmap()ed. But there's no vunmap() done anywhere else in the code for
> > area sections, so who does free it?
> 
> Yes, the error I pointed out is not the only issue with this code.
> 
> You might consider trying my patch which was sent in this thread to Matt
> Gerassimoff.  And if you see anyone whinging that it won't work for
> whatever reason, please ignore them and just test the patch.  You will
> not be disappointed.

Sorry it took so long to test. The patch indeed works fine, thanks! And
the test loop works also fine even without any schedulings, contrary of
what was speculated. (Not that it matters in real usage).

I'll do some more testing and report if I see any oddities.

 Tomi

Re: ioremap()/iounmap() problem

[Matt Gerassimoff]

On Thu, 22 Jan 2009 04:55:13 -0700, Tomi Valkeinen  
<tomi.valkeinen@nokia.com> wrote:

>>
>> You might consider trying my patch which was sent in this thread to Matt
>> Gerassimoff.  And if you see anyone whinging that it won't work for
>> whatever reason, please ignore them and just test the patch.  You will
>> not be disappointed.
>
> Sorry it took so long to test. The patch indeed works fine, thanks! And
> the test loop works also fine even without any schedulings, contrary of
> what was speculated. (Not that it matters in real usage).

I've tested it as well.  I was on a 2.6.28-rc branch initially.  I pulled
the latest linus git sources and tested the patch.  It works.

I stand corrected and now I'll stop whining.

-- 
Matt

RE: ioremap()/iounmap() problem

[Woodruff, Richard]

> Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> with iounmap(). It looks like iounmap() does not properly free large
> areas. Below is a test which fails for me in 6-7 loops.
>
> OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> generic ARM code. I looked at the ioremap code and for larger areas the
> code uses area sections, and I believe the bug is somewhere there.
>
> Does this work on other processors?

If you wait around a bit using some kind of schedule() + jiffy wait after free will it work longer?

Every now and then I've seen some deferred resource releases cause failures of such loops.  Usually they are more obvious like if your really malloc/free'ing direct memory, but there are indirect allocations which might add up.

Anyway, it's a behavioral question to help isolate.

Regards,
Richard W.

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 07:34:47AM -0600, Woodruff, Richard wrote:
> > Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> > with iounmap(). It looks like iounmap() does not properly free large
> > areas. Below is a test which fails for me in 6-7 loops.
> >
> > OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> > generic ARM code. I looked at the ioremap code and for larger areas the
> > code uses area sections, and I believe the bug is somewhere there.
> >
> > Does this work on other processors?
> 
> If you wait around a bit using some kind of schedule() + jiffy wait after
> free will it work longer?
> 
> Every now and then I've seen some deferred resource releases cause
> failures of such loops.  Usually they are more obvious like if your
> really malloc/free'ing direct memory, but there are indirect allocations
> which might add up.
> 
> Anyway, it's a behavioral question to help isolate.

There is no deferred resource releasing here, so such a test is a waste
of time.

RE: ioremap()/iounmap() problem

[Woodruff, Richard]

> On Mon, Jan 19, 2009 at 07:34:47AM -0600, Woodruff, Richard wrote:
> > > Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> > > with iounmap(). It looks like iounmap() does not properly free large
> > > areas. Below is a test which fails for me in 6-7 loops.
> > >
> > > OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> > > generic ARM code. I looked at the ioremap code and for larger areas the
> > > code uses area sections, and I believe the bug is somewhere there.
> > >
> > > Does this work on other processors?
> >
> > If you wait around a bit using some kind of schedule() + jiffy wait after
> > free will it work longer?
> >
> > Every now and then I've seen some deferred resource releases cause
> > failures of such loops.  Usually they are more obvious like if your
> > really malloc/free'ing direct memory, but there are indirect allocations
> > which might add up.
> >
> > Anyway, it's a behavioral question to help isolate.
>
> There is no deferred resource releasing here, so such a test is a waste
> of time.

None?  Aren't at least TLB table structures allocated to cover memory but some other tables?

Or are those all synchronous allocate and release?  I was under some impression that kmalloc's to get some of these came from slabs which don't reap right away.

Defiantly we have had issues in video memory before in these kinds of test loops.

Are you simplifying or am I just off.

Thanks,
Richard W.

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 07:48:09AM -0600, Woodruff, Richard wrote:
> 
> > On Mon, Jan 19, 2009 at 07:34:47AM -0600, Woodruff, Richard wrote:
> > > > Running with latest linux-omap kernel on OMAP3 SDP board, I have problem
> > > > with iounmap(). It looks like iounmap() does not properly free large
> > > > areas. Below is a test which fails for me in 6-7 loops.
> > > >
> > > > OMAP spesific ioremap code doesn't do much, so I think it's somewhere in
> > > > generic ARM code. I looked at the ioremap code and for larger areas the
> > > > code uses area sections, and I believe the bug is somewhere there.
> > > >
> > > > Does this work on other processors?
> > >
> > > If you wait around a bit using some kind of schedule() + jiffy wait after
> > > free will it work longer?
> > >
> > > Every now and then I've seen some deferred resource releases cause
> > > failures of such loops.  Usually they are more obvious like if your
> > > really malloc/free'ing direct memory, but there are indirect allocations
> > > which might add up.
> > >
> > > Anyway, it's a behavioral question to help isolate.
> >
> > There is no deferred resource releasing here, so such a test is a waste
> > of time.
> 
> None?  Aren't at least TLB table structures allocated to cover memory
> but some other tables?
> 
> Or are those all synchronous allocate and release?  I was under some
> impression that kmalloc's to get some of these came from slabs which
> don't reap right away.

There are no L2 PTE tables for section and supersection mappings, so this
is irrelevent.  Section and supersection mappings live in the L1 page
table which persists for the lifetime of the task.

As such, ioremap/iounmap are only allocating and freeing a small structure
to account for the allocation - if things are so tight that 6-7 loops
causes that allocation to fail, you're not going to be able to run any
userspace programs.

This is clearly a bug report which is too vague - which talks about a
"failure" but doesn't say what the failure is.  Until we have more
information from the reporter, uninformed speculation is rather wasteful.

Let's wait until the reporter tells us what the nature of this failure
is, as I've already requested an hour ago.

Re: ioremap()/iounmap() problem

[Matt Gerassimoff]

On Mon, 19 Jan 2009 06:56:54 -0700, Russell King - ARM Linux  
<linux@arm.linux.org.uk> wrote:

> There are no L2 PTE tables for section and supersection mappings, so this
> is irrelevent.  Section and supersection mappings live in the L1 page
> table which persists for the lifetime of the task.
>
> As such, ioremap/iounmap are only allocating and freeing a small  
> structure
> to account for the allocation - if things are so tight that 6-7 loops
> causes that allocation to fail, you're not going to be able to run any
> userspace programs.

This issue is the reason I have subscribed to the mailing list.  I have  
discovered
the problem and had a quick patch to solve it.  Basically when I call  
ioremap() and
then iounmap() (loading and unloading a module) successively, I get a  
different
virtual address each time.  Eventually, it causes an invalid pointer and  
causes
and OOPS.  The solution is not optimal but it may point to the actual  
problem.
The call to ioremap() is a macro that eventually gets to a routine  
__arm_ioremap_pfn
which is implemented in arch/arm/mm/ioremap.c.  This function calls  
get_vm_area()
which calls __get_vm_area_node() (both within mm/vmalloc.c).  Finally,  
there is a
routine called alloc_vmap_area() which allocates and initialized a struct  
vmap_area.
The __iounmap() function (also implemented in arch/arm/mm/ioremap.c) never  
takes the
struct vmap_area into account.   Looking through the sources, it's not  
easy to get
a pointer to that structure from a struct vm_struct pointer.  So I went  
back to
__arm_ioremap_pfn() and found a line that is completely baffling.  I  
#ifdef'd it out
to test and it completely solved the problem.  Here is a simple patch to  
offending
area of code:

---
  arch/arm/mm/ioremap.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 18373f7..e7d2b24 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -297,9 +297,11 @@ __arm_ioremap_pfn(unsigned long pfn, unsigned long  
offset,
                !((__pfn_to_phys(pfn) | size | addr) & ~SUPERSECTION_MASK))  
{
                 area->flags |= VM_ARM_SECTION_MAPPING;
                 err = remap_area_supersections(addr, pfn, size, type);
+#if 0
         } else if (!((__pfn_to_phys(pfn) | size | addr) & ~PMD_MASK)) {
                 area->flags |= VM_ARM_SECTION_MAPPING;
                 err = remap_area_sections(addr, pfn, size, type);
+#endif
         } else
  #endif
                 err = remap_area_pages(addr, pfn, size, type);
--
1.6.0.6

The statement:

	} else if (!((__pfn_to_phys(pfn) | size | addr) & ~PMD_MASK)) {

is the strange one.  I'm not what is being checked here except the  
PMD_MASK.
But without that code, everything works 100%.  I'm not sure what all the  
remap_area_sections()
code does, but the cleanup definitely does not work,  as the kernel OOPS  
will testify.
There may be a better solution, but as far as I can tell, it's not really  
needed.  Maybe
someone else will disagree.

Matt

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 08:06:27AM -0700, Matt Gerassimoff wrote:
> This issue is the reason I have subscribed to the mailing list.  I have  
> discovered the problem and had a quick patch to solve it.

It's not a solution, it's a work-around.  You're not seeing the problem
anymore because you've changed the code to avoid using section mappings.

That's fine if you want to eat lots of TLB entries for your mapping.

> The call to ioremap() is a macro that eventually gets to a routine  
> __arm_ioremap_pfn which is implemented in arch/arm/mm/ioremap.c.
> This function calls get_vm_area() which calls __get_vm_area_node()
> (both within mm/vmalloc.c).

Ok so far.

> Finally, there is a routine called alloc_vmap_area() which allocates and
> initialized a struct vmap_area.

And this is why we now have the problem.  When the ARM section based
ioremap implementation was written, none of that additional code
existed.

The reason we've ended up in this situation is because iounmap() is
doing the _insane_ thing and implementing its own method of freeing
independent of mm/vmalloc.c - this is called "fragile coding" and
must be avoided.

The solution is to fix iounmap() to use proper interfaces into mm/vmalloc.c
when removing the section and supersection mappings.

> The statement:
> 
> 	} else if (!((__pfn_to_phys(pfn) | size | addr) & ~PMD_MASK)) {
> 
> is the strange one.  I'm not what is being checked here except the  
> PMD_MASK.

	(a | b | c) & mask

	(a & mask) | (b & mask) | (c & mask)

	(a & mask) || (b & mask) || (c & mask)

and PMD_MASK is used to mask off the offset into a 2MB section.  So,
(a & ~PMD_MASK) gives the offset into the 2MB section.

So, the test is:

	- is the physical address aligned to 2MB
and
	- is the size aligned to 2MB
and
	- is the virtual address aligned to 2MB
then
	map using section mappings.

> But without that code, everything works 100%.  I'm not sure what all the  
> remap_area_sections()
> code does, but the cleanup definitely does not work,  as the kernel OOPS  
> will testify.
> There may be a better solution, but as far as I can tell, it's not really  
> needed.  Maybe
> someone else will disagree.

We might as well rip this code out then.  I'm all for simpler code, but I'm
sure the folk who want to squeeze the best performance out of their machines
will quickly squeel if I did that.

And what cleanup are you referring to?

Re: ioremap()/iounmap() problem

[Matt Gerassimoff]

On Mon, 19 Jan 2009 08:22:37 -0700, Russell King - ARM Linux  
<linux@arm.linux.org.uk> wrote:

> On Mon, Jan 19, 2009 at 08:06:27AM -0700, Matt Gerassimoff wrote:
>> This issue is the reason I have subscribed to the mailing list.  I have
>> discovered the problem and had a quick patch to solve it.
>
> It's not a solution, it's a work-around.  You're not seeing the problem
> anymore because you've changed the code to avoid using section mappings.

That's what I said at the end of the first message.

> The solution is to fix iounmap() to use proper interfaces into  
> mm/vmalloc.c
> when removing the section and supersection mappings.

After looking at how the the vmap_area's are managed, that's going to
be a tough one.
>
>> The statement:
>>
>> 	} else if (!((__pfn_to_phys(pfn) | size | addr) & ~PMD_MASK)) {
>>
>> is the strange one.  I'm not what is being checked here except the
>> PMD_MASK.
>
> 	(a | b | c) & mask
>
> 	(a & mask) | (b & mask) | (c & mask)
>
> 	(a & mask) || (b & mask) || (c & mask)
>
> and PMD_MASK is used to mask off the offset into a 2MB section.  So,
> (a & ~PMD_MASK) gives the offset into the 2MB section.
>
> So, the test is:
>
> 	- is the physical address aligned to 2MB
> and
> 	- is the size aligned to 2MB
> and
> 	- is the virtual address aligned to 2MB
> then
> 	map using section mappings.

Ok, that makes sense.

>> But without that code, everything works 100%.  I'm not sure what all the
>> remap_area_sections()
>> code does, but the cleanup definitely does not work,  as the kernel OOPS
>> will testify.
>> There may be a better solution, but as far as I can tell, it's not  
>> really
>> needed.  Maybe
>> someone else will disagree.
>
> We might as well rip this code out then.  I'm all for simpler code, but  
> I'm
> sure the folk who want to squeeze the best performance out of their  
> machines
> will quickly squeel if I did that.
>
> And what cleanup are you referring to?

I meant freeing, not cleanup.  Sorry.

As I said it's a start.  I'm all for performance but not at the expense
of integrity.  Right now the code doesn't work and needs to be fixed.

-- 
Matt

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 08:39:39AM -0700, Matt Gerassimoff wrote:
> >>But without that code, everything works 100%.  I'm not sure what all the
> >>remap_area_sections()
> >>code does, but the cleanup definitely does not work,  as the kernel OOPS
> >>will testify.
> >>There may be a better solution, but as far as I can tell, it's not  
> >>really
> >>needed.  Maybe
> >>someone else will disagree.
> >
> >We might as well rip this code out then.  I'm all for simpler code, but  
> >I'm
> >sure the folk who want to squeeze the best performance out of their  
> >machines
> >will quickly squeel if I did that.
> >
> >And what cleanup are you referring to?
> 
> I meant freeing, not cleanup.  Sorry.

Okay, I won't include that bug fix with the fix I'm going to be working
on, even though it's clearly a bug in its own right.  After all, it
"definitely does not work" in your words.

(It may not fix the problem _you're_ seeing, but I assure you, that code
as it stands is wrong.)

> As I said it's a start.  I'm all for performance but not at the expense
> of integrity.  Right now the code doesn't work and needs to be fixed.

Yes, I think everyone realises that there is a bug here which needs
fixing.

Unfortunately, vague "it fails" bug reports are utterly useless for
finding bugs.  Your report provided that extra bit of information
which points to where the real problem lies.

Now, I could be looking at finishing off what I'm currently working on
so I can move on to fixing it instead of writing this message in reply
to an obvious statement.

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 03:58:57PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 19, 2009 at 08:39:39AM -0700, Matt Gerassimoff wrote:
> > >>But without that code, everything works 100%.  I'm not sure what all the
> > >>remap_area_sections()
> > >>code does, but the cleanup definitely does not work,  as the kernel OOPS
> > >>will testify.
> > >>There may be a better solution, but as far as I can tell, it's not  
> > >>really
> > >>needed.  Maybe
> > >>someone else will disagree.
> > >
> > >We might as well rip this code out then.  I'm all for simpler code, but  
> > >I'm
> > >sure the folk who want to squeeze the best performance out of their  
> > >machines
> > >will quickly squeel if I did that.
> > >
> > >And what cleanup are you referring to?
> > 
> > I meant freeing, not cleanup.  Sorry.
> 
> Okay, I won't include that bug fix with the fix I'm going to be working
> on, even though it's clearly a bug in its own right.  After all, it
> "definitely does not work" in your words.
> 
> (It may not fix the problem _you're_ seeing, but I assure you, that code
> as it stands is wrong.)
> 
> > As I said it's a start.  I'm all for performance but not at the expense
> > of integrity.  Right now the code doesn't work and needs to be fixed.
> 
> Yes, I think everyone realises that there is a bug here which needs
> fixing.
> 
> Unfortunately, vague "it fails" bug reports are utterly useless for
> finding bugs.  Your report provided that extra bit of information
> which points to where the real problem lies.
> 
> Now, I could be looking at finishing off what I'm currently working on
> so I can move on to fixing it instead of writing this message in reply
> to an obvious statement.

Right, ignoring everything about "that fix definitely does not work"
here's an as yet untested patch which should fix the problem.

The other thing to point out is that vunmap() does its work lazily
(as Richard tried to point out).  However, it'll become unlazy when
it has more than 32MB * NR_CPUS to deal with.  So, make sure that your
ioremap/vmalloc region is larger than 32MB otherwise you'll still see
crashes.

(That being the space between top of RAM + 8MB, and VMALLOC_END as seen
in Documentation/arm/memory.txt.)

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 18373f7..9f88dd3 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -138,7 +138,7 @@ void __check_kvm_seq(struct mm_struct *mm)
  */
 static void unmap_area_sections(unsigned long virt, unsigned long size)
 {
-	unsigned long addr = virt, end = virt + (size & ~SZ_1M);
+	unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));
 	pgd_t *pgd;
 
 	flush_cache_vunmap(addr, end);
@@ -337,10 +337,7 @@ void __iounmap(volatile void __iomem *io_addr)
 	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
 #ifndef CONFIG_SMP
 	struct vm_struct **p, *tmp;
-#endif
-	unsigned int section_mapping = 0;
 
-#ifndef CONFIG_SMP
 	/*
 	 * If this is a section based mapping we need to handle it
 	 * specially as the VM subsystem does not know how to handle
@@ -352,11 +349,8 @@ void __iounmap(volatile void __iomem *io_addr)
 	for (p = &vmlist ; (tmp = *p) ; p = &tmp->next) {
 		if ((tmp->flags & VM_IOREMAP) && (tmp->addr == addr)) {
 			if (tmp->flags & VM_ARM_SECTION_MAPPING) {
-				*p = tmp->next;
 				unmap_area_sections((unsigned long)tmp->addr,
 						    tmp->size);
-				kfree(tmp);
-				section_mapping = 1;
 			}
 			break;
 		}
@@ -364,7 +358,6 @@ void __iounmap(volatile void __iomem *io_addr)
 	write_unlock(&vmlist_lock);
 #endif
 
-	if (!section_mapping)
-		vunmap(addr);
+	vunmap(addr);
 }
 EXPORT_SYMBOL(__iounmap);

Re: ioremap()/iounmap() problem

[Matt Gerassimoff]

On Mon, 19 Jan 2009 09:13:58 -0700, Russell King - ARM Linux  
<linux@arm.linux.org.uk> wrote:

> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 18373f7..9f88dd3 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -138,7 +138,7 @@ void __check_kvm_seq(struct mm_struct *mm)
>   */
>  static void unmap_area_sections(unsigned long virt, unsigned long size)
>  {
> -	unsigned long addr = virt, end = virt + (size & ~SZ_1M);
> +	unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));
>  	pgd_t *pgd;
> 	flush_cache_vunmap(addr, end);
> @@ -337,10 +337,7 @@ void __iounmap(volatile void __iomem *io_addr)
>  	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
>  #ifndef CONFIG_SMP
>  	struct vm_struct **p, *tmp;
> -#endif
> -	unsigned int section_mapping = 0;
> -#ifndef CONFIG_SMP
>  	/*
>  	 * If this is a section based mapping we need to handle it
>  	 * specially as the VM subsystem does not know how to handle
> @@ -352,11 +349,8 @@ void __iounmap(volatile void __iomem *io_addr)
>  	for (p = &vmlist ; (tmp = *p) ; p = &tmp->next) {
>  		if ((tmp->flags & VM_IOREMAP) && (tmp->addr == addr)) {
>  			if (tmp->flags & VM_ARM_SECTION_MAPPING) {
> -				*p = tmp->next;
>  				unmap_area_sections((unsigned long)tmp->addr,
>  						    tmp->size);
> -				kfree(tmp);
> -				section_mapping = 1;
>  			}
>  			break;
>  		}
> @@ -364,7 +358,6 @@ void __iounmap(volatile void __iomem *io_addr)
>  	write_unlock(&vmlist_lock);
>  #endif
> -	if (!section_mapping)
> -		vunmap(addr);
> +	vunmap(addr);
>  }
>  EXPORT_SYMBOL(__iounmap);

Before this is even tested.  I've went down this path.  You will receive a  
ton
of Bad PMD errors.  unmap_area_sections() and vunmap() don't play nice  
together.
This type of solution will require changes to unmap_area_sections().

-- 
Matt

Re: ioremap()/iounmap() problem

[Russell King - ARM Linux]

On Mon, Jan 19, 2009 at 10:07:13AM -0700, Matt Gerassimoff wrote:
> On Mon, 19 Jan 2009 09:13:58 -0700, Russell King - ARM Linux  
> <linux@arm.linux.org.uk> wrote:
> 
> >diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> >index 18373f7..9f88dd3 100644
> >--- a/arch/arm/mm/ioremap.c
> >+++ b/arch/arm/mm/ioremap.c
> >@@ -138,7 +138,7 @@ void __check_kvm_seq(struct mm_struct *mm)
> >  */
> > static void unmap_area_sections(unsigned long virt, unsigned long size)
> > {
> >-	unsigned long addr = virt, end = virt + (size & ~SZ_1M);
> >+	unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));
> > 	pgd_t *pgd;
> >	flush_cache_vunmap(addr, end);
> >@@ -337,10 +337,7 @@ void __iounmap(volatile void __iomem *io_addr)
> > 	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
> > #ifndef CONFIG_SMP
> > 	struct vm_struct **p, *tmp;
> >-#endif
> >-	unsigned int section_mapping = 0;
> >-#ifndef CONFIG_SMP
> > 	/*
> > 	 * If this is a section based mapping we need to handle it
> > 	 * specially as the VM subsystem does not know how to handle
> >@@ -352,11 +349,8 @@ void __iounmap(volatile void __iomem *io_addr)
> > 	for (p = &vmlist ; (tmp = *p) ; p = &tmp->next) {
> > 		if ((tmp->flags & VM_IOREMAP) && (tmp->addr == addr)) {
> > 			if (tmp->flags & VM_ARM_SECTION_MAPPING) {
> >-				*p = tmp->next;
> > 				unmap_area_sections((unsigned long)tmp->addr,
> > 						    tmp->size);
> >-				kfree(tmp);
> >-				section_mapping = 1;
> > 			}
> > 			break;
> > 		}
> >@@ -364,7 +358,6 @@ void __iounmap(volatile void __iomem *io_addr)
> > 	write_unlock(&vmlist_lock);
> > #endif
> >-	if (!section_mapping)
> >-		vunmap(addr);
> >+	vunmap(addr);
> > }
> > EXPORT_SYMBOL(__iounmap);
> 
> Before this is even tested.  I've went down this path.  You will receive a  
> ton
> of Bad PMD errors.  unmap_area_sections() and vunmap() don't play nice  
> together.
> This type of solution will require changes to unmap_area_sections().

Okay, I've just lost interest in persuing this bug any further.

Re: ioremap()/iounmap() problem

[Matt Gerassimoff]

On Mon, 19 Jan 2009 10:14:11 -0700, Russell King - ARM Linux  
<linux@arm.linux.org.uk> wrote:

> On Mon, Jan 19, 2009 at 10:07:13AM -0700, Matt Gerassimoff wrote:
>> On Mon, 19 Jan 2009 09:13:58 -0700, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>
>> >diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
>> >index 18373f7..9f88dd3 100644
>> >--- a/arch/arm/mm/ioremap.c
>> >+++ b/arch/arm/mm/ioremap.c
>> >@@ -138,7 +138,7 @@ void __check_kvm_seq(struct mm_struct *mm)
>> >  */
>> > static void unmap_area_sections(unsigned long virt, unsigned long  
>> size)
>> > {
>> >-	unsigned long addr = virt, end = virt + (size & ~SZ_1M);
>> >+	unsigned long addr = virt, end = virt + (size & ~(SZ_1M - 1));
>> > 	pgd_t *pgd;
>> >	flush_cache_vunmap(addr, end);
>> >@@ -337,10 +337,7 @@ void __iounmap(volatile void __iomem *io_addr)
>> > 	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
>> > #ifndef CONFIG_SMP
>> > 	struct vm_struct **p, *tmp;
>> >-#endif
>> >-	unsigned int section_mapping = 0;
>> >-#ifndef CONFIG_SMP
>> > 	/*
>> > 	 * If this is a section based mapping we need to handle it
>> > 	 * specially as the VM subsystem does not know how to handle
>> >@@ -352,11 +349,8 @@ void __iounmap(volatile void __iomem *io_addr)
>> > 	for (p = &vmlist ; (tmp = *p) ; p = &tmp->next) {
>> > 		if ((tmp->flags & VM_IOREMAP) && (tmp->addr == addr)) {
>> > 			if (tmp->flags & VM_ARM_SECTION_MAPPING) {
>> >-				*p = tmp->next;
>> > 				unmap_area_sections((unsigned long)tmp->addr,
>> > 						    tmp->size);
>> >-				kfree(tmp);
>> >-				section_mapping = 1;
>> > 			}
>> > 			break;
>> > 		}
>> >@@ -364,7 +358,6 @@ void __iounmap(volatile void __iomem *io_addr)
>> > 	write_unlock(&vmlist_lock);
>> > #endif
>> >-	if (!section_mapping)
>> >-		vunmap(addr);
>> >+	vunmap(addr);
>> > }
>> > EXPORT_SYMBOL(__iounmap);
>>
>> Before this is even tested.  I've went down this path.  You will  
>> receive a
>> ton
>> of Bad PMD errors.  unmap_area_sections() and vunmap() don't play nice
>> together.
>> This type of solution will require changes to unmap_area_sections().
>
> Okay, I've just lost interest in persuing this bug any further.

I mis-spoke, the changes required are in remap_area_sections().  They
are not doing proper manipulations to the pmd information.  If
remap_area_sections() is fixed, there will be no need for  
unmap_area_sections()
as vunmap() will do all the work as far as I can tell.

 From that statement you just made, where does that leave us?  This in my  
estimation
is a MAJOR issue.  If you need help testing this, I'll be happy to do so.   
It may be
faster for you to create a simple kernel module that ioremap() on module  
init and
iounmep() on module exit.  Printing the returned virtual address from  
ioremap() on
each load will yeild a higher and higher addresses until it returns a NULL  
pointer
and kernel OOPS.  I'm running on a at91sam9260 and it will fail on about 6  
to 10
load/unload cycles of the kernel module.  So it happens pretty fast.

-- 
Matt

RE: ioremap()/iounmap() problem

[Woodruff, Richard]

 > The other thing to point out is that vunmap() does its work lazily
> (as Richard tried to point out).  However, it'll become unlazy when
> it has more than 32MB * NR_CPUS to deal with.  So, make sure that your
> ioremap/vmalloc region is larger than 32MB otherwise you'll still see
> crashes.

Where we felt the most problems in past years was in low physical memory situations with this kind of looping.

The system would get caught in a live lock in rebalance code. That code tries to shrink caches but needs some allocations for it to happen.  Maybe 2.6.29 code is better in this regard.

The other common but easier to fix spot was test code where user space mmap callers would spin in alloc and free. The MMAP calls would always fail after a while.

I've not hear reports for a while so maybe current kernel is great here or the tests aren't being run anymore.

Glad RMK has found the root cause for this issue.

Regards,
Richard W.