[lkml]   [2010]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Subject[PATCH 0/5] MIPS: HIGHMEM DMA on noncoherent MIPS32 processors
    I had some time this weekend to revisit an old problem:

    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

    [<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

    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:


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

    #define lock_kmap() spin_lock_irq(&kmap_lock)
    #define lock_kmap() spin_lock(&kmap_lock)

    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:

    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:

    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:

    static inline void flush_dcache_page(struct page *page)
    if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc)


    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.

     \ /
      Last update: 2010-09-07 06:23    [W:0.030 / U:3.804 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site