Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 11:33:07 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v3 1/4] powerpc/mobility: wait for memory transfer to
 complete
To:     benh@kernel.crashing.org, haren@linux.vnet.ibm.com,
        Laurent Dufour <ldufour@linux.ibm.com>, linux@roeck-us.net,
        mpe@ellerman.id.au, nathanl@linux.ibm.com, paulus@samba.org,
        wim@linux-watchdog.org
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        linux-watchdog@vger.kernel.org
References: <20220627135347.32624-1-ldufour@linux.ibm.com>
        <20220627135347.32624-2-ldufour@linux.ibm.com>
In-Reply-To: <20220627135347.32624-2-ldufour@linux.ibm.com>
MIME-Version: 1.0
Message-Id: <1657588908.mis26ebam4.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Laurent Dufour's message of June 27, 2022 11:53 pm:
> In pseries_migration_partition(), loop until the memory transfer is
> complete. This way the calling drmgr process will not exit earlier,
> allowing callbacks to be run only once the migration is fully completed.
>=20
> If reading the VASI state is done after the hypervisor has completed the
> migration, the HCALL is returning H_PARAMETER. We can safely assume that
> the memory transfer is achieved if this happens.
>=20
> This will also allow to manage the NMI watchdog state in the next commits=
.
>=20
> Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/mobility.c | 42 +++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
>=20
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/pla=
tforms/pseries/mobility.c
> index 78f3f74c7056..907a779074d6 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -427,6 +427,43 @@ static int wait_for_vasi_session_suspending(u64 hand=
le)
>  	return ret;
>  }
> =20
> +static void wait_for_vasi_session_completed(u64 handle)
> +{
> +	unsigned long state =3D 0;
> +	int ret;
> +
> +	pr_info("waiting for memory transfert to complete...\n");

                                            ^ extra t (also below)
> +
> +	/*
> +	 * Wait for transition from H_VASI_RESUMED to H_VASI_COMPLETED.
> +	 */
> +	while (true) {
> +		ret =3D poll_vasi_state(handle, &state);
> +
> +		/*
> +		 * If the memory transfer is already complete and the migration
> +		 * has been cleaned up by the hypervisor, H_PARAMETER is return,
> +		 * which is translate in EINVAL by poll_vasi_state().
> +		 */
> +		if (ret =3D=3D -EINVAL || (!ret && state =3D=3D H_VASI_COMPLETED)) {
> +			pr_info("memory transfert completed.\n");
> +			break;
> +		}
> +
> +		if (ret) {
> +			pr_err("H_VASI_STATE return error (%d)\n", ret);
> +			break;
> +		}
> +
> +		if (state !=3D H_VASI_RESUMED) {
> +			pr_err("unexpected H_VASI_STATE result %lu\n", state);
> +			break;
> +		}
> +
> +		msleep(500);

Is 500 specified anywhere? Another caller uses 1000, and the other one=20
uses some backoff interval starting at 1ms...

> +	}
> +}
> +
>  static void prod_single(unsigned int target_cpu)
>  {
>  	long hvrc;
> @@ -673,9 +710,10 @@ static int pseries_migrate_partition(u64 handle)
>  	vas_migration_handler(VAS_SUSPEND);
> =20
>  	ret =3D pseries_suspend(handle);
> -	if (ret =3D=3D 0)
> +	if (ret =3D=3D 0) {
>  		post_mobility_fixup();
> -	else
> +		wait_for_vasi_session_completed(handle);

If this wasn't required until later patches, maybe a comment about why=20
it's here? Could call it wait_for_migration() or similar too.

Looks okay though from my basic reading of PAPR.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> +	} else
>  		pseries_cancel_migration(handle, ret);
> =20
>  	vas_migration_handler(VAS_RESUME);
> --=20
> 2.36.1
>=20
>=20