lkml.org 
[lkml]   [2008]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] MM: Make page tables relocatable -- conditional flush (rc9)
On Mon, Apr 14, 2008 at 6:57 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> This patchset doesn't apply to the 2.6.26 queue because of the ongoing x86
> shell game: the arch/x86/kernel/smp_??.c files were consolidated.

It's probably best to just wait until the smoke clears on 2.6.26 then.
I'll add some comments, however I usually get in trouble for adding
too verbose comments, so I've learned to go the other way. If you
prefer comments though, I'll add them.

> - Must ->page_table_relocation_lock be a semaphore? mutexes are
> preferred.

Not any more. It used to require a semaphore, but I can switch it
back to a mutex now. I can even replace the mutex with an atomic
inc/dec which might be even better since it will work at interrupt
time as well.

> - The patch adds a number of largeish inlined functions. There's rarely
> a need for this, and it can lead to large icache footprint which will, we
> expect, produce slower code.

If these are the ones I'm thinking of, they are in the fast path on
page faults. So they should be inlined. However, I could easily
change it to a small macro or inline function and a regular function
call that would rarely be taken. This should be a win from the icache
point of view and only a loss in a case we really don't care much
about.

> - The patch adds a lot of macros which look like they could have been
> implemented as inlines. Inlines are preferred, please. They look nicer,
> they provide typechecking, they avoid accidental
> multiple-reference-to-arguments bugs and they help to avoid
> unused-variable warnings.

Here I disagree. The only added function-like #define's I see are
either just aliasing functions, or the case when any function that
does nothing. I guess the later could be replaced by inlines to avoid
warnings.

> - Doing PAGE_SIZE memcpy under spin_lock_irqsave() might get a bit
> expensive from an interrupt-latency POV. It could (I think?) result in
> large periods of time where interrupts are almost always disabled, which
> might disrupt some device drivers.

Here I'm just being stupid. There is no reason to have interrupts
disabled at this point.

>
> - Why is this code doing spin_lock_irqsave() on page_table_lock? The
> rest of mm/ doesn't disable IRQs for that lock. This implies that

Laziness. I didn't feel like figuring this out if the irqsave was
necessary when I started, and forgot to go back and fix it later.
There is no reason.


> - I haven't checked, but if the code is taking KM_USER0 from interrupt
> context then that would be a bug. Switching to KM_IRQ0 would fix that.

KM_USER0 is currently correct. For memory hotplug, we may need to
change this in the future.


Ross


\
 
 \ /
  Last update: 2008-04-15 14:53    [W:0.064 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site