Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 02 May 2019 09:12:34 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v4] powerpc/pseries: Remove limit in wait for dying CPU
To:     Thiago Jung Bauermann <bauerman@linux.ibm.com>,
        Nathan Lynch <nathanl@linux.ibm.com>
Cc:     Gautham R Shenoy <ego@linux.vnet.ibm.com>,
        linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        Michael Bringmann <mwb@linux.vnet.ibm.com>,
        Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>,
        Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
References: <20190423223914.3882-1-bauerman@linux.ibm.com>
        <877ebbsb8u.fsf@linux.ibm.com> <87v9yve02x.fsf@morokweng.localdomain>
        <8736lyrzmh.fsf@linux.ibm.com>
In-Reply-To: <8736lyrzmh.fsf@linux.ibm.com>
MIME-Version: 1.0
User-Agent: astroid/0.14.0 (https://github.com/astroidmail/astroid)
Message-Id: <1556752043.jyg2z3kgaw.astroid@bobo.none>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Nathan Lynch's on May 2, 2019 12:57 am:
> Hi Thiago,
>=20
> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>> Nathan Lynch <nathanl@linux.ibm.com> writes:
>>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>>> +		while (true) {
>>>>  			cpu_status =3D smp_query_cpu_stopped(pcpu);
>>>>  			if (cpu_status =3D=3D QCSS_STOPPED ||
>>>>  			    cpu_status =3D=3D QCSS_HARDWARE_ERROR)
>>>>  				break;
>>>> -			cpu_relax();
>>>> +			udelay(100);
>>>>  		}
>>>>  	}
>>>
>>> I agree with looping indefinitely but doesn't it need a cond_resched()
>>> or similar check?
>>
>> If there's no kernel or hypervisor bug, it shouldn't take more than a
>> few tens of ms for this loop to complete (Gautham measured a maximum of
>> 10 ms on a POWER9 with an earlier version of this patch).
>=20
> 10ms is twice the default scheduler quantum...
>=20
>=20
>> In case of bugs related to CPU hotplug (either in the kernel or the
>> hypervisor), I was hoping that the resulting lockup warnings would be a
>> good indicator that something is wrong. :-)
>=20
> Not convinced we should assume something is wrong if it takes a few
> dozen ms to complete the operation.

Right, and if there is no kernel or hypervisor bug then it will stop
eventually :)

> AFAIK we don't have any guarantees
> about the maximum latency of stop-self, and it can be affected by other
> activity in the system, whether we're in shared processor mode, etc. Not
> to mention smp_query_cpu_stopped has to acquire the global RTAS lock and
> be serialized with other tasks calling into RTAS. So I am concerned
> about generating spurious warnings here.

Agreed.

>=20
> If for whatever reason the operation is taking too long, drmgr or
> whichever application is initiating the change will appear to stop
> making progress. It's not too hard to find out what's going on with
> facilities like perf or /proc/pid/stack.
>=20
>=20
>> Though perhaps adding a cond_resched() every 10 ms or so, with a
>> WARN_ON() if it loops for more than 50 ms would be better.
>=20
> A warning doesn't seem appropriate to me, and cond_resched should be
> invoked in each iteration. Or just msleep(1) in each iteration would be
> fine, I think.
>=20
> But I'd like to bring in some more context -- here is the body of
> pseries_cpu_die:
>=20
> static void pseries_cpu_die(unsigned int cpu)
> {
> 	int tries;
> 	int cpu_status =3D 1;
> 	unsigned int pcpu =3D get_hard_smp_processor_id(cpu);
>=20
> 	if (get_preferred_offline_state(cpu) =3D=3D CPU_STATE_INACTIVE) {
> 		cpu_status =3D 1;
> 		for (tries =3D 0; tries < 5000; tries++) {
> 			if (get_cpu_current_state(cpu) =3D=3D CPU_STATE_INACTIVE) {
> 				cpu_status =3D 0;
> 				break;
> 			}
> 			msleep(1);
> 		}
> 	} else if (get_preferred_offline_state(cpu) =3D=3D CPU_STATE_OFFLINE) {
>=20
> 		for (tries =3D 0; tries < 25; tries++) {
> 			cpu_status =3D smp_query_cpu_stopped(pcpu);
> 			if (cpu_status =3D=3D QCSS_STOPPED ||
> 			    cpu_status =3D=3D QCSS_HARDWARE_ERROR)
> 				break;
> 			cpu_relax();
> 		}
> }
>=20
> This patch alters the behavior of the second loop (the CPU_STATE_OFFLINE
> branch). The CPU_STATE_INACTIVE branch is used when the offline behavior
> is to use H_CEDE instead of stop-self, correct?
>=20
> And isn't entering H_CEDE expected to be quite a bit faster than
> stop-self? If so, why does that path get five whole seconds[*] while
> we're bikeshedding about tens of milliseconds for stop-self? :-)
>=20
> [*] And should it be made to retry indefinitely as well?

I think so.

Thanks,
Nick
=