[PATCH 0/5] MIPS: HIGHMEM DMA on noncoherent MIPS32 processors

[Kevin Cernekee <cernekee@gmail.com>]

I had some time this weekend to revisit an old problem:

http://www.linux-mips.org/archives/linux-mips/2010-05/msg00009.html


PROBLEM #1:

When testing Dezhong's patch, I found that un-kmapped pages could get
passed into dma_map_sg().  The original patch did not handle this case.
This resulted in serious DMA coherency issues on my system, along the
lines of "/sbin/init segfaults on boot."

I found that this calling sequence, seen during SATA disk I/O, was the
culprit:

[<8002b3d4>] dma_map_sg+0x2ac/0x2b8                                             
[<802cdb70>] ata_qc_issue+0x274/0x37c                                           
[<802d383c>] ata_scsi_translate+0xb0/0x1e8                                      
[<802d7274>] ata_scsi_queuecmd+0xc8/0x2b8                                       
[<802b0630>] scsi_dispatch_cmd+0x118/0x2b8                                      
[<802b6c64>] scsi_request_fn+0x420/0x4d8                                        
[<80239550>] __blk_run_queue+0x84/0x19c                                         
[<802364a0>] elv_insert+0x158/0x2e4                                             
[<8023a398>] __make_request+0x11c/0x4d0                                         
[<80238144>] generic_make_request+0x348/0x4a0                                   
[<80238330>] submit_bio+0x94/0x13c                                              
[<80114024>] mpage_bio_submit+0x30/0x40                                         
[<80115a2c>] mpage_readpages+0x134/0x170                                        
[<800a4fd4>] __do_page_cache_readahead+0x214/0x314                              

__do_page_cache_readahead() allocates a bunch of pages for the I/O
requests, but I do not see them getting mapped to kernel addresses via
kmap() / kmap_atomic().  This explains why (PageHighMem(page) &&
!kmap_high_get(page)) can be true.

I do not think there is anything wrong with this behavior, so it seems
like dma_map_sg() will need to handle it somehow.  Looking at other
architectures:

The PPC approach (__dma_sync_page_highmem()) is to disable IRQs and call
kmap_atomic() to create a temporary mapping for the page.  Disabling
IRQs is necessary because it is possible (but not required) for the
dma_* functions to be invoked from interrupt context; it also disables
preemption.  kmap_atomic() guarantees a unique mapping per CPU.

Interestingly, PPC does not use kmap_high_get() at all.

The ARM approach (kmap_high_l1_vipt()) recognizes that it is not always
desirable to keep IRQs disabled during DMA flushes, so the ARM
maintainers implemented a sort of "reentrant kmap_atomic()" that allows
multiple contexts to share the same pte by saving and restoring whatever
was there prior to the DMA sync.

Due to its use of kmap_high_get(), the ARM approach suffers from problem
#2, below.  It is also more complex and harder to test.  I'm not sure
how to recreate some of the worst corner cases, e.g. hardirq cacheflush
interrupts a softirq cacheflush which interrupted a user cacheflush.

Dezhong's patch was largely based on the ARM scheme, minus the
kmap_high_l1_vipt() logic.  I am sending an update to this patch which
attempts to imitate the PPC approach instead.  There is a considerable
amount of reuse since both strategies require similar modifications to
dma-default.c, in order to get that code to pass around "struct page"
pointers rather than kseg0 addresses.


PROBLEM #2:

Consider this sequence:

mm/highmem.c:kmap_high()
mm/highmem.c:map_new_virtual()
mm/highmem.c:flush_all_zero_pkmaps()
arch/mips/kernel/smp.c:flush_tlb_kernel_range()
kernel/softirq.c:on_each_cpu()
kernel/smp.c:smp_call_function()

The first thing kmap_high() does is lock_kmap(), which disables
interrupts on kmap_high_get() architectures:

#ifdef ARCH_NEEDS_KMAP_HIGH_GET
#define lock_kmap()             spin_lock_irq(&kmap_lock)
...
#else
#define lock_kmap()             spin_lock(&kmap_lock)
#endif

smp_call_function() may not be called with interrupts disabled:

 * You must not call this function with disabled interrupts or from a
 * hardware interrupt handler or from a bottom half handler.

So, on SMP, we get warnings like:

------------[ cut here ]------------                                            
WARNING: at kernel/smp.c:293 smp_call_function_single+0x17c/0x260()             
Modules linked in:
Call Trace:                                                                     
[<800160f0>] dump_stack+0x8/0x34                                                
[<8004a544>] warn_slowpath_common+0x78/0xa4                                     
[<8004a588>] warn_slowpath_null+0x18/0x24                                       
[<800876e4>] smp_call_function_single+0x17c/0x260                               
[<80087ca8>] smp_call_function+0x28/0x38                                        
[<800531b4>] on_each_cpu+0x1c/0x80                                              
[<80026d20>] flush_tlb_kernel_range+0x28/0x34                                   
[<800b95f4>] kmap_high+0x1dc/0x270                                              
[<8002a570>] __kmap+0x60/0x7c                                                   
[<801f15b4>] do_readpage+0x68/0x540                                             
[<801f218c>] ubifs_write_begin+0xbc/0x53c                                       
[<8009a558>] generic_perform_write+0xd4/0x1f0                                   
[<8009a6e4>] generic_file_buffered_write+0x70/0xbc                              
[<8009dd3c>] __generic_file_aio_write+0x2fc/0x600                               
[<8009e0b0>] generic_file_aio_write+0x70/0xf4                                   
[<800da864>] do_sync_write+0xc4/0x13c                                           
[<800db50c>] vfs_write+0xc0/0x168                                               
[<800db6ac>] sys_write+0x4c/0xa4                                                
[<80003d9c>] stack_done+0x20/0x3c                                               
                                                                                
---[ end trace 8abde6adefbcc81f ]---

I did some digging and found that ARM runs into the same problem.  For
processors that cannot "broadcast" TLB operations, SMP + HIGHMEM are
deemed incompatible:

http://www.spinics.net/lists/arm-kernel/msg74563.html

So, I would opt for the PPC approach in order to avoid this conflict.
Disabling SMP will result in a much nastier performance problem than
blocking interrupts during flushes.


PROBLEM #3:

Regarding David's flush_data_cache_page() concern:

http://www.linux-mips.org/archives/linux-mips/2008-03/msg00011.html


There are at least 4 reasons to flush the cache:

1) Boot time (cache contents are undefined at reset)

2) DMA coherence

3) I$/D$ coherence (self-modifying code)

4) Zap cache aliases


My interpretation of the code is that flush_dcache_page() is only called
for #3 and #4:

#define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 1
static inline void flush_dcache_page(struct page *page)
{               
        if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc)
                __flush_dcache_page(page);
                
}       


The fact that the L2 / board cache is not flushed in c-r4k.c leads me to
believe that #2 is not an intended use of this function:

static inline void local_r4k_flush_data_cache_page(void * addr)
{
        r4k_blast_dcache_page((unsigned long) addr);
}

Besides, we already have a standard, documented DMA API that should be
used instead.

Since this is not boot time, and HIGHMEM is incompatible with cache
aliases, that leaves #3 as a possible issue.  Is this function ever used
for I$/D$ coherence?

I should note that ARM has special handling in __flush_dcache_page() for
high pages.  IIRC, at one point they tried to support HIGHMEM on systems
with cache aliases, before deciding it was not feasible.  Maybe this
code is just a relic - or maybe it's really needed for something.