Re: [PATCH] add missing mutex lock to amdgpu_get_xgmi_hive() (v2)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2019-01-07 11:33 a.m., Grodzovsky, Andrey wrote:
> 
> 
> On 01/07/2019 11:16 AM, Liu, Shaoyun wrote:
>> I think it's reasonable to use the hive  specific lock for hive  specific functions.
>> The changes is acked-by  Shaoyun.liu < Shaoyun.liu@xxxxxxx>
>>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of StDenis, Tom
>> Sent: Monday, January 7, 2019 10:16 AM
>> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: StDenis, Tom <Tom.StDenis@xxxxxxx>
>> Subject: [PATCH] add missing mutex lock to amdgpu_get_xgmi_hive() (v2)
>>
>> v2: Move locks around in other functions so that this function can stand on its own.  Also only hold the hive specific lock for add/remove device instead of the driver global lock so you can't add/remove devices in parallel from one hive.
>>
>> Signed-off-by: Tom St Denis <tom.stdenis@xxxxxxx>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 +-
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c   | 36 ++++++++++++++--------
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h   |  2 +-
>>    3 files changed, 25 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 39d5d058b2c7..13d8e2ad2f7a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3525,7 +3525,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>    	 * by different nodes. No point also since the one node already executing
>>    	 * reset will also reset all the other nodes in the hive.
>>    	 */
>> -	hive = amdgpu_get_xgmi_hive(adev);
>> +	hive = amdgpu_get_xgmi_hive(adev, 0);
>>    	if (hive && adev->gmc.xgmi.num_physical_nodes > 1 &&
>>    	    !mutex_trylock(&hive->hive_lock))
>>    		return 0;
> 
> Let's say i have device 0 in hive A and it just got a gpu reset and at
> the same time device 1 is being added to same have though
> amdgpu_xgmi_add_device, hive->hive_lock is acquired by this new device
> being added and so gpu reset for device 0 bails out on
> '!mutex_trylock(&hive->hive_lock))' without completing the reset.
> Also in general i feel a bit uncomfortable about the confusing
> acquisition scheme in the function and  the fact that you take the
> hive->hive_lock inside amdgpu_get_xgmi_hive but release is still outside
> of the function.

Is adding a device while resetting a device even a valid operation 
anyways?

I think this means more so that the reset logic is broken.  Instead 
there should be a per-hive reset lock that is taken and that is tested 
instead.

Tom


> 
> Andrey
> 
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> index 8a8bc60cb6b4..ebf50809485f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> @@ -40,26 +40,39 @@ void *amdgpu_xgmi_hive_try_lock(struct amdgpu_hive_info *hive)
>>    	return &hive->device_list;
>>    }
>>    
>> -struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev)
>> +struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device
>> +*adev, int lock)
>>    {
>>    	int i;
>>    	struct amdgpu_hive_info *tmp;
>>    
>>    	if (!adev->gmc.xgmi.hive_id)
>>    		return NULL;
>> +
>> +	mutex_lock(&xgmi_mutex);
>> +
>>    	for (i = 0 ; i < hive_count; ++i) {
>>    		tmp = &xgmi_hives[i];
>> -		if (tmp->hive_id == adev->gmc.xgmi.hive_id)
>> +		if (tmp->hive_id == adev->gmc.xgmi.hive_id) {
>> +			if (lock)
>> +				mutex_lock(&tmp->hive_lock);
>> +			mutex_unlock(&xgmi_mutex);
>>    			return tmp;
>> +		}
>>    	}
>> -	if (i >= AMDGPU_MAX_XGMI_HIVE)
>> +	if (i >= AMDGPU_MAX_XGMI_HIVE) {
>> +		mutex_unlock(&xgmi_mutex);
>>    		return NULL;
>> +	}
>>    
>>    	/* initialize new hive if not exist */
>>    	tmp = &xgmi_hives[hive_count++];
>>    	tmp->hive_id = adev->gmc.xgmi.hive_id;
>>    	INIT_LIST_HEAD(&tmp->device_list);
>>    	mutex_init(&tmp->hive_lock);
>> +	if (lock)
>> +		mutex_lock(&tmp->hive_lock);
>> +
>> +	mutex_unlock(&xgmi_mutex);
>>    
>>    	return tmp;
>>    }
>> @@ -111,8 +124,8 @@ int amdgpu_xgmi_add_device(struct amdgpu_device *adev)
>>    		return ret;
>>    	}
>>    
>> -	mutex_lock(&xgmi_mutex);
>> -	hive = amdgpu_get_xgmi_hive(adev);
>> +	/* find hive and take lock */
>> +	hive = amdgpu_get_xgmi_hive(adev, 1);
>>    	if (!hive)
>>    		goto exit;
>>    
>> @@ -142,8 +155,8 @@ int amdgpu_xgmi_add_device(struct amdgpu_device *adev)
>>    			break;
>>    	}
>>    
>> +	mutex_unlock(&hive->hive_lock);
>>    exit:
>> -	mutex_unlock(&xgmi_mutex);
>>    	return ret;
>>    }
>>    
>> @@ -154,15 +167,12 @@ void amdgpu_xgmi_remove_device(struct amdgpu_device *adev)
>>    	if (!adev->gmc.xgmi.supported)
>>    		return;
>>    
>> -	mutex_lock(&xgmi_mutex);
>> -
>> -	hive = amdgpu_get_xgmi_hive(adev);
>> +	hive = amdgpu_get_xgmi_hive(adev, 1);
>>    	if (!hive)
>> -		goto exit;
>> +		return;
>>    
>>    	if (!(hive->number_devices--))
>>    		mutex_destroy(&hive->hive_lock);
>> -
>> -exit:
>> -	mutex_unlock(&xgmi_mutex);
>> +	else
>> +		mutex_unlock(&hive->hive_lock);
>>    }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>> index 6151eb9c8ad3..8d7984844174 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>> @@ -32,7 +32,7 @@ struct amdgpu_hive_info {
>>    	struct mutex hive_lock;
>>    };
>>    
>> -struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev);
>> +struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device
>> +*adev, int lock);
>>    int amdgpu_xgmi_update_topology(struct amdgpu_hive_info *hive, struct amdgpu_device *adev);  int amdgpu_xgmi_add_device(struct amdgpu_device *adev);  void amdgpu_xgmi_remove_device(struct amdgpu_device *adev);
>> --
>> 2.17.2
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux