| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150304152515.GL15405@linux.vnet.ibm.com>
Date: Wed, 4 Mar 2015 07:25:15 -0800
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc: linux-kernel@...r.kernel.org, mingo@...nel.org,
laijs@...fujitsu.com, dipankar@...ibm.com,
akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
josh@...htriplett.org, tglx@...utronix.de, peterz@...radead.org,
rostedt@...dmis.org, dhowells@...hat.com, edumazet@...gle.com,
dvhart@...ux.intel.com, fweisbec@...il.com, oleg@...hat.com,
bobby.prani@...il.com, x86@...nel.org,
Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
David Vrabel <david.vrabel@...rix.com>,
xen-devel@...ts.xenproject.org
Subject: Re: [PATCH tip/core/rcu 02/20] x86: Use common
outgoing-CPU-notification code
On Wed, Mar 04, 2015 at 09:55:11AM -0500, Boris Ostrovsky wrote:
> On 03/04/2015 09:43 AM, Paul E. McKenney wrote:
> >On Tue, Mar 03, 2015 at 02:31:51PM -0800, Paul E. McKenney wrote:
> >>On Tue, Mar 03, 2015 at 05:06:50PM -0500, Boris Ostrovsky wrote:
> >>>On 03/03/2015 04:26 PM, Paul E. McKenney wrote:
> >>>>On Tue, Mar 03, 2015 at 03:13:07PM -0500, Boris Ostrovsky wrote:
> >>>>>On 03/03/2015 02:42 PM, Paul E. McKenney wrote:
> >>>>>>On Tue, Mar 03, 2015 at 02:17:24PM -0500, Boris Ostrovsky wrote:
> >>>>>>>On 03/03/2015 12:42 PM, Paul E. McKenney wrote:
> >>>>>>>> }
> >>>>>>>>@@ -511,7 +508,8 @@ static void xen_cpu_die(unsigned int cpu)
> >>>>>>>> schedule_timeout(HZ/10);
> >>>>>>>> }
> >>>>>>>>- cpu_die_common(cpu);
> >>>>>>>>+ (void)cpu_wait_death(cpu, 5);
> >>>>>>>>+ /* FIXME: Are the below calls really safe in case of timeout? */
> >>>>>>>Not for HVM guests (PV guests will only reach this point after
> >>>>>>>target cpu has been marked as down by the hypervisor).
> >>>>>>>
> >>>>>>>We need at least to have a message similar to what native_cpu_die()
> >>>>>>>prints on cpu_wait_death() failure. And I think we should not call
> >>>>>>>the two routines below (three, actually --- there is also
> >>>>>>>xen_teardown_timer() below, which is not part of the diff).
> >>>>>>>
> >>>>>>>-boris
> >>>>>>>
> >>>>>>>
> >>>>>>>> xen_smp_intr_free(cpu);
> >>>>>>>> xen_uninit_lock_cpu(cpu);
> >>>>>>So something like this, then?
> >>>>>>
> >>>>>> if (cpu_wait_death(cpu, 5)) {
> >>>>>> xen_smp_intr_free(cpu);
> >>>>>> xen_uninit_lock_cpu(cpu);
> >>>>>> xen_teardown_timer(cpu);
> >>>>>> }
> >>>>> else
> >>>>> pr_err("CPU %u didn't die...\n", cpu);
> >>>>>
> >>>>>
> >>>>>>Easy change for me to make if so!
> >>>>>>
> >>>>>>Or do I need some other check for HVM-vs.-PV guests, and, if so, what
> >>>>>>would that check be? And also if so, is it OK to online a PV guest's
> >>>>>>CPU that timed out during its previous offline?
> >>>>>I believe PV VCPUs will always be CPU_DEAD by the time we get here
> >>>>>since we are (indirectly) waiting for this in the loop at the
> >>>>>beginning of xen_cpu_die():
> >>>>>
> >>>>>'while (xen_pv_domain() && HYPERVISOR_vcpu_op(VCPUOP_is_up, cpu,
> >>>>>NULL))' will exit only after 'HYPERVISOR_vcpu_op(VCPUOP_down,
> >>>>>smp_processor_id()' in xen_play_dead(). Which happens after
> >>>>>play_dead_common() has marked the cpu as CPU_DEAD.
> >>>>>
> >>>>>So no test is needed.
> >>>>OK, so I have the following patch on top of my previous patch, which
> >>>>I will merge if testing goes well. So if a CPU times out going offline,
> >>>>the above three functions will not be called, the "didn't die" message
> >>>>will be printed, and any future attempt to online that CPU will fail.
> >>>>Is that the correct semantics?
> >>>Yes.
> >>>
> >>>I am not sure whether not ever onlining the CPU is the best outcome
> >>>but then I don't think trying to online it again with all interrupts
> >>>and such still set up will work well. And it's an improvement over
> >>>what we have now anyway (with current code we may clean up things
> >>>for a non-dead cpu).
> >>Another strategy is to key off of the return value of cpu_check_up_prepare().
> >>If it returns -EBUSY, then the outgoing CPU finished up after the
> >>surviving CPU timed out. The CPU trying to bring the new CPU online
> >>could (in theory, anyway) do the xen_smp_intr_free(), xen_uninit_lock_cpu(),
> >>and xen_teardown_timer() at that point.
> >And the code for this, in xen_cpu_up(), might look something like the
> >following:
> >
> > rc = cpu_check_up_prepare(cpu);
> > if (rc && rc != -EBUSY)
> > return rc;
> > if (rc == EBUSY) {
> > xen_smp_intr_free(cpu);
> > xen_uninit_lock_cpu(cpu);
> > xen_teardown_timer(cpu);
> > }
> >
> >The idea is that we detect when the CPU eventually took itself offline,
> >but only did so after the surviving CPU timed out. (Of course, it
> >would probably be best to put those three statements into a small
> >function that is called from both places.)
> >
> >I have no idea whether this approach would really work, especially given
> >your earlier statement that CPU_DEAD happens early on. But in case it
> >is helpful or sparks some better idea.
>
> Let me test this, I think it may work.
>
> In the meantime, it turned out that HVM guests are broken by this
> patch (with our without changes that we've been discussing), because
> HVM CPUs die with
>
> static void xen_hvm_cpu_die(unsigned int cpu)
> {
> xen_cpu_die(cpu);
> native_cpu_die(cpu);
> }
>
> Which means that cpu_wait_death() is called twice, and second call
> moves the CPU to CPU_BROKEN.
Yikes! I did miss this one. :-(
> The simple solution is to stop calling native_cpu_die() above but
> I'd like to use common code in native_cpu_die(). I'll see if I can
> carve it out without too much damage to x86.
Very good, thank you! I look forward to seeing your patch.
Thanx, Paul
> Thanks.
> -boris
>
>
> >
> > Thanx, Paul
> >
> >>But I must defer to you on this sort of thing.
> >>
> >> Thanx, Paul
> >>
> >>>Thanks.
> >>>-boris
> >>>
> >>>
> >>>> Thanx, Paul
> >>>>
> >>>>------------------------------------------------------------------------
> >>>>
> >>>>diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> >>>>index e2c7389c58c5..f2a06ff0614d 100644
> >>>>--- a/arch/x86/xen/smp.c
> >>>>+++ b/arch/x86/xen/smp.c
> >>>>@@ -508,12 +508,13 @@ static void xen_cpu_die(unsigned int cpu)
> >>>> schedule_timeout(HZ/10);
> >>>> }
> >>>>- (void)cpu_wait_death(cpu, 5);
> >>>>- /* FIXME: Are the below calls really safe in case of timeout? */
> >>>>-
> >>>>- xen_smp_intr_free(cpu);
> >>>>- xen_uninit_lock_cpu(cpu);
> >>>>- xen_teardown_timer(cpu);
> >>>>+ if (cpu_wait_death(cpu, 5)) {
> >>>>+ xen_smp_intr_free(cpu);
> >>>>+ xen_uninit_lock_cpu(cpu);
> >>>>+ xen_teardown_timer(cpu);
> >>>>+ } else {
> >>>>+ pr_err("CPU %u didn't die...\n", cpu);
> >>>>+ }
> >>>> }
> >>>> static void xen_play_dead(void) /* used only with HOTPLUG_CPU */
> >>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists