From: ebiederm@xmission.com (Eric W. Biederman)
To: Gleb Natapov <gleb@redhat.com>
Cc: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>,
        "x86\@kernel.org" <x86@kernel.org>,
        "kexec\@lists.infradead.org" <kexec@lists.infradead.org>,
        Marcelo Tosatti <mtosatti@redhat.com>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kvm\@vger.kernel.org" <kvm@vger.kernel.org>
References: <50ADE0C2.1000106@cn.fujitsu.com> <50ADE11A.401@cn.fujitsu.com>
	<87ip8sxuyh.fsf@xmission.com> <20121126172054.GF12969@redhat.com>
	<87fw3wuuoh.fsf@xmission.com> <20121126175327.GG12969@redhat.com>
Date: Mon, 26 Nov 2012 12:18:27 -0600
In-Reply-To: <20121126175327.GG12969@redhat.com> (Gleb Natapov's message of
	"Mon, 26 Nov 2012 19:53:27 +0200")
Message-ID: <87mwy4teh8.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH v8 1/2] x86/kexec: add a new atomic notifier list for kdump
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2904
Lines: 65

Gleb Natapov <gleb@redhat.com> writes:

> On Mon, Nov 26, 2012 at 11:43:10AM -0600, Eric W. Biederman wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Mon, Nov 26, 2012 at 09:08:54AM -0600, Eric W. Biederman wrote:
>> >> Zhang Yanfei <zhangyanfei@cn.fujitsu.com> writes:
>> >> 
>> >> > This patch adds an atomic notifier list named crash_notifier_list.
>> >> > Currently, when loading kvm-intel module, a notifier will be registered
>> >> > in the list to enable vmcss loaded on all cpus to be VMCLEAR'd if
>> >> > needed.
>> >> 
>> >> crash_notifier_list ick gag please no.  Effectively this makes the kexec
>> >> on panic code path undebuggable.
>> >> 
>> >> Instead we need to use direct function calls to whatever you are doing.
>> >> 
>> > The code walks linked list in kvm-intel module and calls vmclear on
>> > whatever it finds there. Since the function have to resides in kvm-intel
>> > module it cannot be called directly. Is callback pointer that is set
>> > by kvm-intel more acceptable?
>> 
>> Yes a specific callback function is more acceptable.  Looking a little
>> deeper vmclear_local_loaded_vmcss is not particularly acceptable. It is
>> doing a lot of work that is unnecessary to save the virtual registers
>> on the kexec on panic path.
>> 
> What work are you referring to in particular that may not be
> acceptable?

The unnecessary work that I was see is all of the software state
changing.  Unlinking things from linked lists flipping variables.
None of that appears related to the fundamental issue saving cpu
state.

Simply reusing a function that does more than what is strictly required
makes me nervous.  What is the chance that the function will grow
with maintenance and add constructs that are not safe in a kexec on
panic situtation.

>> In fact I wonder if it might not just be easier to call vmcs_clear to a
>> fixed per cpu buffer.
>> 
> There may be more than one vmcs loaded on a cpu, hence the list.
>
>> Performing list walking in interrupt context without locking in
>> vmclear_local_loaded vmcss looks a bit scary.  Not that locking would
>> make it any better, as locking would simply add one more way to deadlock
>> the system.  Only an rcu list walk is at all safe.  A list walk that
>> modifies the list as vmclear_local_loaded_vmcss does is definitely not safe.
>> 
> The list vmclear_local_loaded walks is per cpu. Zhang's kvm patch
> disables kexec callback while list is modified.

If the list is only modified on it's cpu and we are running on that cpu
that does look like it will give the necessary protections.  It isn't
particularly clear at first glance that is the case unfortunately.

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