Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplug_request()

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 3/1/21 11:03 PM, David Gibson wrote:
> On Fri, Feb 26, 2021 at 01:32:58PM -0300, Daniel Henrique Barboza wrote:
>> Now that we're asserting the first DRC LMB earlier, use it to query if
>> the DRC is already pending unplug and, in this case, issue the same
>> error we already do.
>>
>> The previous check was introduced in commit 2a129767ebb1 and it works,
>> but it's easier to check the unplug_requested  flag instead of looking
>> for the existence of the sPAPRDIMMState. It's also compliant with what
>> is already done in other unplug_request functions for other devices.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> I'm having some trouble completely convincing myself this is right.
> 
> What about this situation:
>       1. We initiate a DIMM unplug
>       	- unplug_request is set on all the LMBs
> 	- all the LMBs go on the pending_unplug list
>       2. The guest encounters no problems, and starts issuing set
>          indicator calls to mark the LMBs unusable, starting from the
> 	lowest address
>       3. On drc_set_unusable() for the first LMB, we see that unplug is
>          requested and call spapr_drc_release()
>       4. spapr_drc_release() on the first LMB clears unplug_requested
>       5. At this point, but before this is done on *all* of the DIMM's
>          LMBs, the user attempts another unplug triggering the code
> 	below
> 
> AFAICT this will now skip the error, since the first LMB is no longer
> in unplug_requested state, but there *are* still pending unplugs for
> some of the remaining LMBs, so the old code would have tripped the
> error.

Good point. Checking the existence of the sPAPRDIMMState struct at this
point is the same as checking for drc->unplug_requested for all the DRCs
of the DIMM.

I could check for drc->unplug_requested inside the loop where we instantiate
each DRC, but there is no gain in doing that instead of what we already have.

I'll drop this patch and change patch 1 to just remove the duplicated assert.


Thanks,


DHB

> 
>> ---
>>   hw/ppc/spapr.c | 8 +-------
>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 74e046b522..149dc2113f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3681,13 +3681,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>>                                   addr_start / SPAPR_MEMORY_BLOCK_SIZE);
>>       g_assert(drc_start);
>>   
>> -    /*
>> -     * An existing pending dimm state for this DIMM means that there is an
>> -     * unplug operation in progress, waiting for the spapr_lmb_release
>> -     * callback to complete the job (BQL can't cover that far). In this case,
>> -     * bail out to avoid detaching DRCs that were already released.
>> -     */
>> -    if (spapr_pending_dimm_unplugs_find(spapr, dimm)) {
>> +    if (spapr_drc_unplug_requested(drc_start)) {
>>           error_setg(errp, "Memory unplug already in progress for device %s",
>>                      dev->id);
>>           return;
>