On Fri, 06 Apr 2007 17:02:58 -0700 Jeremy Fitzhardinge <jeremy@xxxxxxxx> wrote:
> Andrew Morton wrote:
> > All this paravirt stuff isn't making the kernel any prettier, is it?
> >
>
> You're too kind. wli's comment on the first version of this patch was
> something along the lines of "this patch causes a surprising amount of
> damage for what little it achieves".
Damn, I wish I'd said that.
> >> ...
> >>
> >> -#ifndef CONFIG_X86_PAE
> >> -void vmalloc_sync_all(void)
> >> +void _vmalloc_sync_all(void)
> >> {
> >> /*
> >> * Note that races in the updates of insync and start aren't
> >> @@ -600,6 +599,8 @@ void vmalloc_sync_all(void)
> >> static DECLARE_BITMAP(insync, PTRS_PER_PGD);
> >> static unsigned long start = TASK_SIZE;
> >> unsigned long address;
> >> +
> >> + BUG_ON(SHARED_KERNEL_PMD);
> >>
> >> BUILD_BUG_ON(TASK_SIZE & ~PGDIR_MASK);
> >> for (address = start; address >= TASK_SIZE; address += PGDIR_SIZE) {
> >> @@ -623,4 +624,3 @@ void vmalloc_sync_all(void)
> >> start = address + PGDIR_SIZE;
> >> }
> >> }
> >>
> >
> > This is a functional change for non-paravirt kernels. Non-PAE kernels now
> > get a vmalloc_sync_all(). How come?
> >
>
> You mean PAE kernels get a vmalloc_sync_all?
err, yes.
> When we're in PAE mode, but SHARED_KERNEL_PMD is false (which is true
> for Xen, but not for native execution), then the kernel mappings are not
> implicitly shared between processes. This means that the vmalloc
> mappings are not shared, and so need to be explicitly synchronized
> between pagetables, like in the !PAE case.
head spins.
> > Your change clashes pretty fundamantally with
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm4/broken-out/move-die-notifier-handling-to-common-code-fix-vmalloc_sync_all.patch,
> > and
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm4/broken-out/move-die-notifier-handling-to-common-code.patch
> > _does_ make the kernel prettier.
> >
>
> Hm, it doesn't look like a deep clash. Dropping the inline function and
> just putting the "if (SHARED_KERNEL_PMD) return;" at the start of the
> real vmalloc_sync_all() would work fine.
Something like that. I don't want to redo my patch if we're going to change
your patch ;)
> And I like vmalloc_sync_all() being a non-arch-specific interface; it
> cleans up another of the xen patches.
OK.
> > But I'm a bit reluctant to rework
> > move-die-notifier-handling-to-common-code-fix-vmalloc_sync_all.patch
> > (somehow) until I understand why your patch is a) futzing with non-PAE,
> > non-paravirt code
>
> There should be no functional difference for non-paravirt code, PAE or
> non-PAE.
>
> > and b) overengineered.
> >
>
> Overall, or just this bit?
this bit.
> > Why didn't you just stick a
> >
> > if (SHARED_KERNEL_PMD)
> > return;
> >
> > into vmalloc_sync_all()?
> >
>
> That would work, but when building !PARAVIRT && PAE, SHARED_KERNEL_PMD
> is just constant 1, so it would end up making a pointless function
> call. With the wrapper, the call disappears entirely. It probably
> doesn't matter, but I didn't want anyone to complain about making the
> !PARAVIRT generated code worse (hi, Ingo!).
vmalloc_sync_all() is a) tremendously slow and b) only called by
register_die_notifier(). We can afford to add a few cycles to it.
> However, if you're making vmalloc_sync_all a weak function anyway, then
> there's no difference with the paravirt patches in place. The
>
> if (SHARED_KERNEL_PMD)
> return;
>
> will evaluate to
>
> if (1)
> return;
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
