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;
Message-ID: <b820274e-56fe-501b-0d1d-41703fbe6b48@linux.ibm.com>
Date:   Tue, 12 Jul 2022 11:38:43 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.0.1
Subject: Re: [PATCH v3 1/4] powerpc/mobility: wait for memory transfer to
 complete
Content-Language: fr
To:     Nicholas Piggin <npiggin@gmail.com>, benh@kernel.crashing.org,
        haren@linux.vnet.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>
 <1657588908.mis26ebam4.astroid@bobo.none>
From:   Laurent Dufour <ldufour@linux.ibm.com>
In-Reply-To: <1657588908.mis26ebam4.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le 12/07/2022 à 03:33, Nicholas Piggin a écrit :
> 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.
>>
>> 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.
>>
>> This will also allow to manage the NMI watchdog state in the next commits.
>>
>> 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(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/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 handle)
>>  	return ret;
>>  }
>>  
>> +static void wait_for_vasi_session_completed(u64 handle)
>> +{
>> +	unsigned long state = 0;
>> +	int ret;
>> +
>> +	pr_info("waiting for memory transfert to complete...\n");
> 
>                                             ^ extra t (also below)

I tried to push one French word, but you caught it ;)
Will fix that and the other ones.

>> +
>> +	/*
>> +	 * Wait for transition from H_VASI_RESUMED to H_VASI_COMPLETED.
>> +	 */
>> +	while (true) {
>> +		ret = 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 == -EINVAL || (!ret && state == 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 != 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 
> uses some backoff interval starting at 1ms...

This is a bit empiric, the idea is to wait for the overall memory transfer
to be done. There is no real need to interact immediately after the
operation is terminated, so I pick that value to not make too many Hcalls
just for that. From the test I did, that seems to be a reasonable choice.

> 
>> +	}
>> +}
>> +
>>  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);
>>  
>>  	ret = pseries_suspend(handle);
>> -	if (ret == 0)
>> +	if (ret == 0) {
>>  		post_mobility_fixup();
>> -	else
>> +		wait_for_vasi_session_completed(handle);
> 
> If this wasn't required until later patches, maybe a comment about why 
> 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>

Thanks Nick for reviewing this series.

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