Re: [RFC] atomic highmem kmap page pinning

[Minchan Kim]

It seems Andrea's mail address is changed.
I will resend new Andrea's mail address.

On Thu, Mar 5, 2009 at 9:25 AM, Minchan Kim <minchan.kim@xxxxxxxxx> wrote:
> - Show quoted text -
> On Wed, 4 Mar 2009 23:46:33 +0000
> Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> wrote:
>
>> On Thu, Mar 05, 2009 at 08:07:17AM +0900, Minchan Kim wrote:
>> > On Wed, 04 Mar 2009 12:26:00 -0500 (EST)
>> > Nicolas Pitre <nico@xxxxxxx> wrote:
>> >
>> > > On Wed, 4 Mar 2009, Minchan Kim wrote:
>> > >
>> > > > On Wed, 04 Mar 2009 00:58:13 -0500 (EST)
>> > > > Nicolas Pitre <nico@xxxxxxx> wrote:
>> > > >
>> > > > > I've implemented highmem for ARM. ÂYes, some ARM machines do have lots
>> > > > > of memory...
>> > > > >
>> > > > > The problem is that most ARM machines have a non IO coherent cache,
>> > > > > meaning that the dma_map_* set of functions must clean and/or invalidate
>> > > > > the affected memory manually. ÂAnd because the majority of those
>> > > > > machines have a VIVT cache, the cache maintenance operations must be
>> > > > > performed using virtual addresses.
>> > > > >
>> > > > > In dma_map_page(), an highmem pages could still be mapped and cached
>> > > > > even after kunmap() was called on it. ÂAs long as highmem pages are
>> > > > > mapped, page_address(page) is non null and we can use that to
>> > > > > synchronize the cache.
>> > > > > It is unlikely but still possible for kmap() to race and recycle the
>> > > > > obtained virtual address above, and use it for another page though. ÂIn
>> > > > > that case, the new mapping could end up with dirty cache lines for
>> > > > > another page, and the unsuspecting cache invalidation loop in
>> > > > > dma_map_page() won't notice resulting in data loss. ÂHence the need for
>> > > > > some kind of kmap page pinning which can be used in any context,
>> > > > > including IRQ context.
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> > > > > This is a RFC patch implementing the necessary part in the core code, as
>> > > > > suggested by RMK. Please comment.
>> > > >
>> > > > I am not sure if i understand your concern totally.
>> > > > I can understand it can be recycled. but Why is it racing ?
>> > >
>> > > Suppose this sequence of events:
>> > >
>> > > Â - dma_map_page(..., DMA_FROM_DEVICE) is called on a highmem page.
>> > >
>> > > Â --> Â Â - vaddr = page_address(page) is non null. In this case
>> > > Â Â Â Â Â Â it is likely that the page has valid cache lines
>> > > Â Â Â Â Â Â associated with vaddr. Remember that the cache is VIVT.
>> > >
>> > > Â Â Â Â Â --> Â Â - for (i = vaddr; i < vaddr + PAGE_SIZE; i += 32)
>> > > Â Â Â Â Â Â Â Â Â Â Â Â Â invalidate_cache_line(i);
>> > >
>> > > Â *** preemption occurs in the middle of the loop above ***
>> > >
>> > > Â - kmap_high() is called for a different page.
>> > >
>> > > Â --> Â Â - last_pkmap_nr wraps to zero and flush_all_zero_pkmaps()
>> > > Â Â Â Â Â Â is called. ÂThe pkmap_count value for the page passed
>> > > Â Â Â Â Â Â to dma_map_page() above happens to be 1, so it is
>> > > Â Â Â Â Â Â unmapped. ÂBut prior to that, flush_cache_kmaps()
>> > > Â Â Â Â Â Â cleared the cache for it. ÂSo far so good.
>> >
>> > Thanks for kind explanation.:)
>> >
>> > I thought kmap and dma_map_page usage was following.
>> >
>> > kmap(page);
>> > ...
>> > dma_map_page(...)
>> > Â invalidate_cache_line
>> >
>> > kunmap(page);
>>
>> No, that's not the usage at all. Âkmap() can't be called from the
>> contexts which dma_map_page() is called from (iow, IRQ contexts as
>> pointed out in the paragraph I underlined above.)
>>
>> We're talking about dma_map_page() _internally_ calling kmap_get_page()
>> to _atomically_ and _safely_ check whether the page was kmapped. ÂIf
>> it was kmapped, we need to pin the page and return its currently mapped
>> address for cache handling and then release that reference.
>
> Thanks, Russel.
> I see. That was thing I missed. :)
>
>> None of the existing kmap support comes anywhere near to providing a
>> mechanism for this because it can't be used in the contexts under which
>> dma_map_page() is called.
>
> Right.
>
>> If we could do it with existing interfaces, we wouldn't need a new
>> interface would we?
>
> OK.
> As previous said, I don't like kmap_high's irq disable.
> It's already used in many place. so irq'disable effect might be rather big.
>
> How about new interface which is like KM_IRQ's kmap_atomic slot
> Âwith serializing kmap_atomic_lock?
>
> Let's Cced other experts.
>
> --
> Kinds Regards
> Minchan Kim
>



-- 
Kinds regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/