8xx: map_page() skip pinned region and tlbie debugging aid

Dan: I dont think ioremap() is an issue because it never works inside the kernel's static virtual address space (which is the only one we're interested in having pinned at the moment).

source "drivers/Kconfig"

arch/ppc/kernel/misc.S +2 -0

@@ -565,6 +565,19 @@ _GLOBAL(_tlbie)
 	SYNC_601
 	isync
 #else /* CONFIG_SMP */
+#ifdef CONFIG_DEBUG_PIN_TLBIE
+/* check if the address being invalidated overlaps with the pinned 

region */

arch/ppc/kernel/misc.S +3 -0

+	lis	r4,(pin_area_start)@ha
+	lwz	r5,(pin_area_start)@l(4)
+	cmplw	r3, r5

Quoted 5 lines · Marcelo Tosatti

Dan: I dont think ioremap() is an issue because it never works inside the
kernel's static virtual address space (which is the only one we're interested
in having pinned at the moment).

Take a close look at the initialization code. I believe it also pins the IMMR space, which is subject to ioremap().

Quoted 20 lines

source "drivers/Kconfig"

arch/ppc/kernel/misc.S +2 -0

@@ -565,6 +565,19 @@ _GLOBAL(_tlbie)
 	SYNC_601
 	isync
 #else /* CONFIG_SMP */
+#ifdef CONFIG_DEBUG_PIN_TLBIE
+/* check if the address being invalidated overlaps with the pinned 

region */

arch/ppc/kernel/misc.S +9 -0

+	lis	r4,(pin_area_start)@ha
+	lwz	r5,(pin_area_start)@l(4)
+	cmplw	r3, r5
+	blt	11f
+	lis	r4,(pin_area_end)@ha
+	lwz	r5,(pin_area_end)@l(4)
+	cmplw	r3, r5
+	bge	11f
+	trap

Quoted 4 lines

+#endif
+11:
tlbie r3
sync

We don't need this kind of assembly code on the 8xx. Just define _tlbie as a macro (which has always been done) and write this debug stuff as C code.

arch/ppc/kernel/misc.S

Quoted 4 lines

arch/ppc/kernel/misc.S +4 -0

+#ifdef CONFIG_PIN_TLB
+unsigned long pin_area_start = KERNELBASE;
+unsigned long pin_area_end = KERNELBASE + 0x00800000;
+#endif

This only covers the kernel instruction space. We pin 24M bytes of data plus 8M bytes of IMMR.

arch/ppc/kernel/misc.S

Quoted 4 lines

arch/ppc/kernel/misc.S +4 -0

+#ifdef CONFIG_PIN_TLB
+			if (va < pin_area_start || va >= pin_area_end)
+#endif
+				flush_HPTE(0, va, pmd_val(*pd));

We really want to see this generate an error. We shouldn't be calling this on any of the pinned spaces. In the case of initially mapping the kernel space, we should set up the page tables but not call this far down that we get here.

Hi Dan,

Quoted 9 lines · Dan Malek

Quoted 5 lines · Marcelo Tosatti

Dan: I dont think ioremap() is an issue because it never works inside the
kernel's static virtual address space (which is the only one we're interested
in having pinned at the moment).

Quoted 2 lines

Take a close look at the initialization code. I believe it also pins the IMMR space, which is subject to ioremap().

OK. Now that makes me think that the IMMR pinned entry is also always thrashed by the tlbie at map_page() :(

The IMMR space is a 16kB window (correct?), so I wonder if it might be better to the use occupied pinned slot for another more accessed region (an 8MB one preferably!).

Bullshit, map_page()'s call for flush_HPTE() is conditioned by "mem_init_done", and mapin_ram() is called before mem_init_done is set.

Quoted 3 lines

The IMMR space is a 16kB window (correct?), so I wonder if it might be better to the use occupied pinned slot for another more accessed region (an 8MB one preferably!).

Thats still some food for thought...

You see, this just keeps growing in features and complexity :-) It would be nice for ioremap() to consider multiple, dynamic 8M pages on 8xx like it does BATs on traditional PPC. It will do this .... someday soon.

Perhaps I should go back and push some of this dynamic 8M page stuff. It would eliminate the tlbie() problems, give us more flexibility. Damn, not enough hours in a day ....

Hi,

I happen to notice a race condition in the mmu_context code for the 8xx with very few context (16 MMU contexts) and kernel preemption enable. It is hard to reproduce has it shows only when many processes are created/destroy and the system is doing a lot of IRQ processing.

In short, one process is trying to steal a context that is in the process of being freed (mm->context == NO_CONTEXT) but not completely freed (nr_free_contexts == 0).
The steal_context() function does not do anything and the process stays in the loop forever.

Anyway, I got a patch that fixes this part. Does not seem to affect scheduling latency at all.

Comments are appreciated.

Quoted 7 lines · Guillaume Autran

Hi,

I happen to notice a race condition in the mmu_context code for the 8xx with very few context (16 MMU contexts) and kernel preemption enable. It is hard to reproduce has it shows only when many processes are created/destroy and the system is doing a lot of IRQ processing.

In short, one process is trying to steal a context that is in the process of being freed (mm->context == NO_CONTEXT) but not completely freed (nr_free_contexts == 0). The steal_context() function does not do anything and the process stays in the loop forever.

Quoted 5 lines

Anyway, I got a patch that fixes this part. Does not seem to affect scheduling latency at all.

Comments are appreciated.

Your patch seems to do a hell lot more than fixing this race ... What about just calling preempt_disable() in destroy_context() instead ?

Quoted 19 lines · Benjamin Herrenschmidt

Quoted 16 lines · Guillaume Autran

Hi,

I happen to notice a race condition in the mmu_context code for the 8xx with very few context (16 MMU contexts) and kernel preemption enable. It is hard to reproduce has it shows only when many processes are created/destroy and the system is doing a lot of IRQ processing.

In short, one process is trying to steal a context that is in the process of being freed (mm->context == NO_CONTEXT) but not completely freed (nr_free_contexts == 0).
The steal_context() function does not do anything and the process stays in the loop forever.

Anyway, I got a patch that fixes this part. Does not seem to affect scheduling latency at all.

Quoted 8 lines

Quoted 3 lines

Comments are appreciated.

Your patch seems to do a hell lot more than fixing this race ... What about just calling preempt_disable() in destroy_context() instead ?

I'm still a bit confused with "kernel preemption". One thing for sure is that disabling kernel preemption does indeed fix my problem. So, my question is, what if a task in the middle of being schedule gets preempted by an IRQ handler, where will this task restart execution ? Back at the beginning of schedule or where it left of ?

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

Hi Guillaume,

Quoted 29 lines · Guillaume Autran

Quoted 25 lines · Benjamin Herrenschmidt

Quoted 14 lines · Guillaume Autran

Hi,

I happen to notice a race condition in the mmu_context code for the 8xx with very few context (16 MMU contexts) and kernel preemption enable. It is hard to reproduce has it shows only when many processes are created/destroy and the system is doing a lot of IRQ processing.

In short, one process is trying to steal a context that is in the process of being freed (mm->context == NO_CONTEXT) but not completely freed (nr_free_contexts == 0). The steal_context() function does not do anything and the process stays in the loop forever.

Anyway, I got a patch that fixes this part. Does not seem to affect scheduling latency at all.

Comments are appreciated.

Your patch seems to do a hell lot more than fixing this race ... What about just calling preempt_disable() in destroy_context() instead ?

Quoted 4 lines

I'm still a bit confused with "kernel preemption". One thing for sure is that disabling kernel preemption does indeed fix my problem.
So, my question is, what if a task in the middle of being schedule gets preempted by an IRQ handler, where will this task restart execution ? Back at the beginning of schedule or where it left of ?

Execution is resumed exactly where it has been interrupted.

Quoted 3 lines

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

Apparently its there to avoid the spinlock exactly on !FEW_CONTEXTS machines.

I suppose that what happens is that get_mmu_context() gets preempted after stealing a context (so nr_free_contexts = 0), but before setting next_mmu_context to the next entry

next_mmu_context = (ctx + 1) & LAST_CONTEXT;

So if the now running higher prio tasks calls switch_mm() (which is likely to happen) it loops forever on atomic_dec_if_positive(&nr_free_contexts), while steal_context() sees "mm->context == CONTEXT".

I think that you should try "preempt_disable()/preempt_enable" pair at entry and exit of get_mmu_context() - I suppose around destroy_context() is not enough (you can try that also).

spinlock ends up calling preempt_disable().

In that case, what happen when a higher priority task steal the context of the lower priority task after get_mmu_context() but before set_mmu_context() ?
Then when the lower priority task resumes, its context may no longer be valid...
Do I get this right ?

I'm going to do like this instead of my previous attempt:

       /* Setup new userspace context */
       preempt_disable();
       get_mmu_context(next);
       set_context(next->context, next->pgd);
       preempt_enable();

To make sure we don't loose our context in between.

Execution is resumed exactly where it has been interrupted.

Quoted 2 lines

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

Apparently its there to avoid the spinlock exactly on !FEW_CONTEXTS machines.

I suppose that what happens is that get_mmu_context() gets preempted after stealing a context (so nr_free_contexts = 0), but before setting next_mmu_context to the next entry

next_mmu_context = (ctx + 1) & LAST_CONTEXT;

So if the now running higher prio tasks calls switch_mm() (which is likely to happen) it loops forever on atomic_dec_if_positive(&nr_free_contexts), while steal_context() sees "mm->context == CONTEXT".

I think that you should try "preempt_disable()/preempt_enable" pair at entry and exit of get_mmu_context() - I suppose around destroy_context() is not enough (you can try that also).

spinlock ends up calling preempt_disable().

Quoted 8 lines

Execution is resumed exactly where it has been interrupted.

Quoted 2 lines

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

Apparently its there to avoid the spinlock exactly on !FEW_CONTEXTS machines.

Quoted 5 lines

I suppose that what happens is that get_mmu_context() gets preempted after stealing a context (so nr_free_contexts = 0), but before setting next_mmu_context to the next entry

next_mmu_context = (ctx + 1) & LAST_CONTEXT;

Ugh ? Can switch_mm() be preempted at all ? Did I miss yet another "let's open 10 gazillion races for gun" Ingo patch ?

Quoted 3 lines

So if the now running higher prio tasks calls switch_mm() (which is likely to happen) it loops forever on atomic_dec_if_positive(&nr_free_contexts), while steal_context() sees "mm->context == CONTEXT".

I think the race is only when destroy_context() is preempted, but maybe I missed something.

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 9 lines

Execution is resumed exactly where it has been interrupted.

Quoted 3 lines

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

Apparently its there to avoid the spinlock exactly on !FEW_CONTEXTS machines.

Quoted 10 lines

Quoted 6 lines

I suppose that what happens is that get_mmu_context() gets preempted after stealing a context (so nr_free_contexts = 0), but before setting next_mmu_context to the next entry

next_mmu_context = (ctx + 1) & LAST_CONTEXT;

Ugh ? Can switch_mm() be preempted at all ? Did I miss yet another "let's open 10 gazillion races for gun" Ingo patch ?

Doh nope it can't - my bad.

Quoted 9 lines

Quoted 5 lines

So if the now running higher prio tasks calls switch_mm() (which is likely to happen) it loops forever on atomic_dec_if_positive(&nr_free_contexts), while steal_context() sees "mm->context == CONTEXT".

I think the race is only when destroy_context() is preempted, but maybe I missed something.

Nope, I think you are right. My "theory" is obviously flawed now.

There seem to be several contexts where destroy_context() could be called with preempt enabled - I should have been shutup in the first place :)

Lets wait for Guillaume to test...

Well, disabling preemption in the get_mmu_context() does not help much... I'm trying to disable preemption only inside destroy_mmu_context() as suggested. Will keep you posted.

Guillaume.

Quoted 46 lines · Marcelo Tosatti

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 9 lines

Execution is resumed exactly where it has been interrupted.

Quoted 3 lines

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

Apparently its there to avoid the spinlock exactly on !FEW_CONTEXTS machines.

Quoted 10 lines

Quoted 6 lines

I suppose that what happens is that get_mmu_context() gets preempted after stealing a context (so nr_free_contexts = 0), but before setting next_mmu_context to the next entry

next_mmu_context = (ctx + 1) & LAST_CONTEXT;

Ugh ? Can switch_mm() be preempted at all ? Did I miss yet another "let's open 10 gazillion races for gun" Ingo patch ?

Doh nope it can't - my bad.

Quoted 9 lines

Quoted 5 lines

So if the now running higher prio tasks calls switch_mm() (which is likely to happen) it loops forever on atomic_dec_if_positive(&nr_free_contexts), while steal_context() sees "mm->context == CONTEXT".

I think the race is only when destroy_context() is preempted, but maybe I missed something.

Nope, I think you are right. My "theory" is obviously flawed now.

There seem to be several contexts where destroy_context() could be called with preempt enabled - I should have been shutup in the first place :)

Lets wait for Guillaume to test...

Signature · 14 lines

=======================================
Guillaume Autran
Senior Software Engineer
MRV Communications, Inc.
Tel: (978) 952-4932 office
E-mail: [email protected]
======================================= 

------------------------------------------------------------------------

_______________________________________________
Linuxppc-embedded mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Quoted 8 lines

Execution is resumed exactly where it has been interrupted.

Quoted 2 lines

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

Apparently its there to avoid the spinlock exactly on !FEW_CONTEXTS machines.

Quoted 5 lines

I suppose that what happens is that get_mmu_context() gets preempted after stealing a context (so nr_free_contexts = 0), but before setting next_mmu_context to the next entry

next_mmu_context = (ctx + 1) & LAST_CONTEXT;

Ugh ? Can switch_mm() be preempted at all ? Did I miss yet another "let's open 10 gazillion races for gun" Ingo patch ?

I'm still a bit confused with "kernel preemption". One thing for sure is that disabling kernel preemption does indeed fix my problem. So, my question is, what if a task in the middle of being schedule gets preempted by an IRQ handler, where will this task restart execution ? Back at the beginning of schedule or where it left of ?

The idea behind my patch was to get rid of that nr_free_contexts counter that is (I thing) redundant with the context_map.

You should consider 8Mb pages the way we do BATs yes,

Note that I'll soon send the patch I told you about that makes the virtual address picked by io_block_mapping() dynamic, so we no longer have to do crappy assumptions all over the place :)

Quoted 1 line · Benjamin Herrenschmidt

You should consider 8Mb pages the way we do BATs yes,

It's always been considered, just never fully implemented :-)

Quoted 3 lines

Note that I'll soon send the patch I told you about that makes the virtual address picked by io_block_mapping() dynamic, so we no longer have to do crappy assumptions all over the place :)

Whatever, I'll never use it that way and no one else should either.

All of the io_block_mapping() calls should be used to set these 8M

mapped IO spaces, everyone should use ioremap() to map them, and ioremap() has to be modified to find them for the 8xx.

Quoted 1 line · Benjamin Herrenschmidt

You should consider 8Mb pages the way we do BATs yes,

It's always been considered, just never fully implemented :-)

Quoted 3 lines

Note that I'll soon send the patch I told you about that makes the virtual address picked by io_block_mapping() dynamic, so we no longer have to do crappy assumptions all over the place :)

Whatever, I'll never use it that way and no one else should either.

All of the io_block_mapping() calls should be used to set these 8M mapped IO spaces, everyone should use ioremap() to map them, and ioremap() has to be modified to find them for the 8xx.

Once the physical->virtual mapping for device IO space are set with io_block_mapping() (or with ioremap() for dynamic virtual addresses), why would you want to ioremap() the physical address again???

PS: I've had a quick try at converting the IMMAP to use ioremap instead (and have that dynamic virtual address stored in a pointer), changed drivers to use that pointer instead of hardcoded "IMMAP". Didnt work immediately :) Its not that the idea?

- The debate between Dan and me here is about the semantics of
io_block_mapping().

Quoted 2 lines · Benjamin Herrenschmidt

- The debate between Dan and me here is about the semantics of
io_block_mapping().

My point of discussion is this function needs to be much smarter than simply allocating a virtual address space. We need to track the calls so that we can "grow" previous spaces. A single io_block_mapping() should not always allocate a new BAT, CAM or otherwise wired entry. It has to know the alignment, size and amount of resource available. For example, if an io_block_mapping() requests a 4M space, and it isn't possible to wire such a size, we still need to keep track of that such that a subsequent 4M request is combined into a space that can be wired with an 8M entry. We need to make it smart enough to coalesce the spaces to maximize the use of the available and minimal mapping resources. If io_block_mapping() is just a simple functions that decrements a pointer and sets a value in a register, then you have already required the caller to know everything about the mapping details, so why bother performing "hidden" arithmetic that is likely to be known by the caller? If we are going to change this

let's make it tru
y useful, so it understands the capabilities of the processor, optimizes the resources, and keeps generic mapping information so ioremap() doesn't care if it is mapped by BATs, CAMs, or large pages.

Quoted 6 lines · Dan Malek

Quoted 2 lines · Benjamin Herrenschmidt

- The debate between Dan and me here is about the semantics of io_block_mapping().

Quoted 15 lines

My point of discussion is this function needs to be much smarter than simply allocating a virtual address space. We need to track the calls so that we can "grow" previous spaces. A single io_block_mapping() should not always allocate a new BAT, CAM or otherwise wired entry. It has to know the alignment, size and amount of resource available. For example, if an io_block_mapping() requests a 4M space, and it isn't possible to wire such a size, we still need to keep track of that such that a subsequent 4M request is combined into a space that can be wired with an 8M entry. We need to make it smart enough to coalesce the spaces to maximize the use of the available and minimal mapping resources. If io_block_mapping() is just a simple functions that decrements a pointer and sets a value in a register, then you have already required the caller to know everything about the mapping details, so why bother performing "hidden" arithmetic that is likely to be known by the caller? If we are going to change this

Everyting ... but the virtual address, which is quite a bit :) My problem is really with virtual addresses beeing hard coded, which makes things complicated every time we try to do something with the kenrel virtual space. But ....

Quoted 5 lines

let's make it tru
y useful, so it understands the capabilities of the processor, optimizes the resources, and keeps generic mapping information so ioremap() doesn't care if it is mapped by BATs, CAMs, or large pages.

... I do agree that making it even smarter so it can coalesce block
mappings with the same attributes would be "interesting".

Ben.

What do you mean "everyone should use ioremap() to map them"?

Once the physical->virtual mapping for device IO space are set with io_block_mapping() (or with ioremap() for dynamic virtual addresses), why would you want to ioremap() the physical address again???

PS: I've had a quick try at converting the IMMAP to use ioremap instead (and have that dynamic virtual address stored in a pointer), changed drivers to use that pointer instead of hardcoded "IMMAP". Didnt work immediately :) Its not that the idea?

We really want to see this generate an error. We shouldn't be calling this on any of the pinned spaces. In the case of initially mapping the kernel space, we should set up the page tables but not call this far down that we get here.

it yields

_tlbie on pinned range: c0000000-c1800000
Badness in _tlbie at arch/ppc/mm/pgtable.c:527

Call trace:
 [c0005530] dump_stack+0x18/0x28
 [c0003628] check_bug_trap+0x84/0xac
 [c00037b0] ProgramCheckException+0x160/0x1a0
 [c0002d50] ret_from_except_full+0x0/0x4c
 [c000a91c] _tlbie+0x94/0xa0
 [c902f018] alloc_init_module+0x18/0x40 [alloc]
 [c002c4ac] sys_init_module+0x224/0x324
 [c00026f0] ret_from_syscall+0x0/0x44

Quoted 2 lines · Marcelo Tosatti

it yields

Quoted 11 lines

_tlbie on pinned range: c0000000-c1800000
Badness in _tlbie at arch/ppc/mm/pgtable.c:527

Call trace:
[c0005530] dump_stack+0x18/0x28
[c0003628] check_bug_trap+0x84/0xac
[c00037b0] ProgramCheckException+0x160/0x1a0
[c0002d50] ret_from_except_full+0x0/0x4c
[c000a91c] _tlbie+0x94/0xa0
[c902f018] alloc_init_module+0x18/0x40 [alloc]
[c002c4ac] sys_init_module+0x224/0x324
[c00026f0] ret_from_syscall+0x0/0x44

How much real memory on your board?

We need to ensure VMALLOC_START is beyond
the pinned entries.

We should make all of the code
much smarter to pin on the real space that is on
the board.

For testing now, just make VMALLOC_OFFSET 32M, which will push the start to the 32M boundary after the kernel.

Note: this was just a test module doing tlbie(0xc0000100)... Hum, it should also print out the address in question...

Quoted 1 line

How much real memory on your board?

128M

Quoted 2 lines

We need to ensure VMALLOC_START is beyond
the pinned entries.

Right now VMALLOC_START is before the IMMR pinned space.

Oh! What are the side effects of such pinning as the code is today?

For what purpose? Sorry I don't get you, please be more verbose.