kmap_atomic and preemption

kmap_atomic and preemption

[Vineet Gupta]

Hi,

I was staring at some recent ARC highmem crashes and see that kmap_atomic()
disables preemption even when page is in lowmem and call returns right away.
This seems to be true for other arches as well.

arch/arc/mm/highmem.c:

void *kmap_atomic(struct page *page)
{
	int idx, cpu_idx;
	unsigned long vaddr;

	preempt_disable();
	pagefault_disable();
	if (!PageHighMem(page))
		return page_address(page);

        /* do the highmem foo ... */
..
}

I would really like to implement a inline fastpath for !PageHighMem(page) case and
do the highmem foo out-of-line.

Is preemption disabling a requirement of kmap_atomic() callers independent of
where page is or is it only needed when page is in highmem and can trigger page
faults or TLB Misses between kmap_atomic() and kunmap_atomic and wants protection
against reschedules etc.

-Vineet

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Peter Zijlstra]

On Wed, May 04, 2016 at 04:07:40PM +0530, Vineet Gupta wrote:
> Hi,
> 
> I was staring at some recent ARC highmem crashes and see that kmap_atomic()
> disables preemption even when page is in lowmem and call returns right away.
> This seems to be true for other arches as well.
> 
> arch/arc/mm/highmem.c:
> 
> void *kmap_atomic(struct page *page)
> {
> 	int idx, cpu_idx;
> 	unsigned long vaddr;
> 
> 	preempt_disable();
> 	pagefault_disable();
> 	if (!PageHighMem(page))
> 		return page_address(page);
> 
>         /* do the highmem foo ... */
> ..
> }
> 
> I would really like to implement a inline fastpath for !PageHighMem(page) case and
> do the highmem foo out-of-line.
> 
> Is preemption disabling a requirement of kmap_atomic() callers independent of
> where page is or is it only needed when page is in highmem and can trigger page
> faults or TLB Misses between kmap_atomic() and kunmap_atomic and wants protection
> against reschedules etc.

Traditionally kmap_atomic() disables preemption; and the reason is that
the returned pointer must stay valid. This had a side effect in that it
also disabled pagefaults.

We've since de-coupled the pagefault from the preemption thing, so you
could disable pagefaults while leaving preemption enabled.

Now, I've also done preemptible kmap_atomic() on -rt; which appears to
work, suggesting nothing relies on it disabling preemption (on -rt).

So sure; you can try and leave preemption enabled for lowmem pages, see
what comes apart -- if anything. It gives weird semantics for
kmap_atomic() though, and I'm not sure the cost of doing that
preempt_disable/preempt_enable() is worth the pain.

If you want a fast-slow path splt, you can easily do something like:


static inline void *kmap_atomic(struct page *page)
{
	preempt_disable();
	pagefault_disable();
	if (!PageHighMem(page))
		return page_address(page);

	return __kmap_atomic(page);
}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Thomas Petazzoni]

Hello,

On Wed, 4 May 2016 15:47:29 +0200, Peter Zijlstra wrote:

> static inline void *kmap_atomic(struct page *page)
> {
> 	preempt_disable();
> 	pagefault_disable();
> 	if (!PageHighMem(page))
> 		return page_address(page);
> 
> 	return __kmap_atomic(page);
> }

This is essentially what has been done on ARM in commit
9ff0bb5ba60638a688a46e93df8c5009896672eb, showing a pretty significant
improvement in network workloads.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Vineet Gupta]

On Wednesday 04 May 2016 07:23 PM, Thomas Petazzoni wrote:
> Hello,
>
> On Wed, 4 May 2016 15:47:29 +0200, Peter Zijlstra wrote:
>
>> static inline void *kmap_atomic(struct page *page)
>> {
>> 	preempt_disable();
>> 	pagefault_disable();
>> 	if (!PageHighMem(page))
>> 		return page_address(page);
>>
>> 	return __kmap_atomic(page);
>> }
> This is essentially what has been done on ARM in commit
> 9ff0bb5ba60638a688a46e93df8c5009896672eb, showing a pretty significant
> improvement in network workloads.

ARC already has that semantically - only not inline ! I really want to avoid 2
needless LD-ADD-ST for the disabling of preemption and page fault for the low mem
pages by returning early !

static inline void *kmap_atomic(struct page *page)
{
        if (!PageHighMem(page))
		return page_address(page);

	preempt_disable();
	pagefault_disable();
	
	return __kmap_atomic(page);

}

>
> Best regards,
>
> Thomas


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Vineet Gupta]

On Wednesday 04 May 2016 07:17 PM, Peter Zijlstra wrote:
> On Wed, May 04, 2016 at 04:07:40PM +0530, Vineet Gupta wrote:
>> Is preemption disabling a requirement of kmap_atomic() callers independent of
>> where page is or is it only needed when page is in highmem and can trigger page
>> faults or TLB Misses between kmap_atomic() and kunmap_atomic and wants protection
>> against reschedules etc.
> Traditionally kmap_atomic() disables preemption; and the reason is that
> the returned pointer must stay valid. This had a side effect in that it
> also disabled pagefaults.

But how could the ptr possibly get invalid. Say despite the disable calls, we
could actually take the page fault (or TLB Miss on ARC) - the pagefault_disable()
only makes do_page_fault() do reduced handling vs. calling handle_mm_fault() etc.
It is essentially restricting the fault handling to a kernel mode fixup only.

Now if we didn't do disable, on ARC the semantics of do_page_fault() are still the
same - since the address would be for fixmap which is handled under "kernel" only
category as well.

void do_page_fault(unsigned long address, struct pt_regs *regs)
{

    if (address >= VMALLOC_START) {
        ret = handle_kernel_vaddr_fault(address);
        return;
...
    if (faulthandler_disabled() || !mm)
        goto no_context;
...

> We've since de-coupled the pagefault from the preemption thing, so you
> could disable pagefaults while leaving preemption enabled.

Right - I've seen that patch set from David H.

> ...
>
> If you want a fast-slow path splt, you can easily do something like:
>
> static inline void *kmap_atomic(struct page *page)
> {
> 	preempt_disable();
> 	pagefault_disable();
> 	if (!PageHighMem(page))
> 		return page_address(page);
>
> 	return __kmap_atomic(page);
> }

I actually want to return early for !PageHighMem and avoid the pointless 2
LD-ADD-ST to memory for map and 2 LD-SUB-ST for unmap for regular pages for such
cases.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Peter Zijlstra]

On Wed, May 04, 2016 at 02:16:11PM +0000, Vineet Gupta wrote:
> > static inline void *kmap_atomic(struct page *page)
> > {
> > 	preempt_disable();
> > 	pagefault_disable();
> > 	if (!PageHighMem(page))
> > 		return page_address(page);
> >
> > 	return __kmap_atomic(page);
> > }
> 
> I actually want to return early for !PageHighMem and avoid the pointless 2
> LD-ADD-ST to memory for map and 2 LD-SUB-ST for unmap for regular pages for such
> cases.

So I'm fairly sure people rely on the fact you cannot have pagefault
inside a kmap_atomic().

But you could potentially get away with leaving preemption enabled. Give
it a try, see if something goes *bang* ;-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Russell King - ARM Linux]

On Wed, May 04, 2016 at 03:47:29PM +0200, Peter Zijlstra wrote:
> Traditionally kmap_atomic() disables preemption; and the reason is that
> the returned pointer must stay valid. This had a side effect in that it
> also disabled pagefaults.

A lowmem page should never change its page_address(), so that much is
safe.  I think the question is whether there is any driver code which
assumes that preemption is unconditionally disabled between a
kmap_atomic() has been called.

That wouldn't be an unreasonable assumption given the name of the
function, so I'd suggest caution with making kmap_atomic() have these
kinds of differing behaviours depending on whether we're asking to
kmap a high or lowmem page.

If we are going to allow this, I think it at least needs to be well
documented.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Peter Zijlstra]

On Wed, May 04, 2016 at 08:17:55PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 04, 2016 at 03:47:29PM +0200, Peter Zijlstra wrote:
> > Traditionally kmap_atomic() disables preemption; and the reason is that
> > the returned pointer must stay valid. This had a side effect in that it
> > also disabled pagefaults.
> 
> A lowmem page should never change its page_address(), so that much is
> safe. 

Agreed..

> I think the question is whether there is any driver code which
> assumes that preemption is unconditionally disabled between a
> kmap_atomic() has been called.

right, this and consistency. Having the function disable preemption
sometimes is just plain weird.

> That wouldn't be an unreasonable assumption given the name of the
> function, so I'd suggest caution with making kmap_atomic() have these
> kinds of differing behaviours depending on whether we're asking to
> kmap a high or lowmem page.

So for -rt I did a preemptible kmap_atomic() for x86 (and maybe someone
did other archs too, I cannot remember), now -rt is funny wrt locking
anyway, but I cannot remember anything breaking because of this, so
there is some hope it will actually work.

> If we are going to allow this, I think it at least needs to be well
> documented.

Indeed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kmap_atomic and preemption

[Vineet Gupta]

On Wednesday 04 May 2016 08:31 PM, Peter Zijlstra wrote:
> So I'm fairly sure people rely on the fact you cannot have pagefault
> inside a kmap_atomic().

So this translates to: any hardware page faults inside kmap_atomic() can't lead to
do_page_fault() taking a lock - those can only be ex_table fixups, yes ?
Could you please also help explain your earlier comment about kmap_atomic needing
to disable preemption so that "returned pointer stayed valid". I can't quite
fathom how that can happen

> But you could potentially get away with leaving preemption enabled. Give
> it a try, see if something goes *bang* ;-)

So tried patch further below: on a quad core slowish FPGA setup, concurrent
hackbench and LMBench seem to run w/o issues  - so it is not obviously broken even
if not proven otherwise. But the point is highmem page is a slow path anyways -
needs a PTE update, new TLB entry etc. I hoped to not wiggle even a single cache
line for the low page - but seems like that is not possible.

OTOH, if we do keep the status quo - then making these 2 cache lines into 1 is not
possible either. From reading the orig "decoupling of prremption and page fault"
thread it seems to be because preempt count is per cpu on x86.

@@ -67,7 +67,6 @@ void *kmap_atomic(struct page *page)
        int idx, cpu_idx;
        unsigned long vaddr;
 
-       preempt_disable();
        pagefault_disable();
        if (!PageHighMem(page))
                return page_address(page);
@@ -107,7 +106,6 @@ void __kunmap_atomic(void *kv)
        }
 
        pagefault_enable();
-       preempt_enable();
 }


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>