Re: [EXTERNAL] [PATCH 1/2] drm/amdkfd: Cleanup IO links during KFD device removal

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

 




> On Apr 12, 2022, at 8:07 AM, Felix Kuehling <felix.kuehling@xxxxxxx> wrote:
> 
> Am 2022-04-08 um 04:45 schrieb Shuotao Xu:
>> Currently, the IO-links to the device being removed from topology,
>> are not cleared. As a result, there would be dangling links left in
>> the KFD topology. This patch aims to fix the following:
>> 1. Cleanup all IO links to the device being removed.
>> 2. Ensure that node numbering in sysfs and nodes proximity domain
>>    values are consistent after the device is removed:
>>    a. Adding a device and removing a GPU device are made mutually
>>       exclusive.
>>    b. The global proximity domain counter is no longer required to be
>>       an atomic counter. A normal 32-bit counter can be used instead.
>> 3. Update generation_count to let user-mode know that topology has
>>    changed due to device removal.
>> 
>> Reviewed-by: Shuotao Xu <shuotaoxu@xxxxxxxxxxxxx>
>> CC: Shuotao Xu <shuotaoxu@xxxxxxxxxxxxx>
>> Signed-off-by: Mukul Joshi <mukul.joshi@xxxxxxx>
> 
> This looks like Mukul's patch, but with you as the author (otherwise I would have expected a "From: Mukul ..." line at the start of the email). Did you make any changes to it?
> 
> Regards,
>   Felix

Yes it was Mukul’s patch.Was trying to from a patch series, but I could did it wrong.

Regards,
Shuotao
> 
> 
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c     |  4 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h     |  2 +
>>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 79 ++++++++++++++++++++---
>>  3 files changed, 74 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>> index 1eaabd2cb41b..afc8a7fcdad8 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>> @@ -1056,7 +1056,7 @@ static int kfd_parse_subtype_iolink(struct crat_subtype_iolink *iolink,
>>  	 * table, add corresponded reversed direction link now.
>>  	 */
>>  	if (props && (iolink->flags & CRAT_IOLINK_FLAGS_BI_DIRECTIONAL)) {
>> -		to_dev = kfd_topology_device_by_proximity_domain(id_to);
>> +		to_dev = kfd_topology_device_by_proximity_domain_no_lock(id_to);
>>  		if (!to_dev)
>>  			return -ENODEV;
>>  		/* same everything but the other direction */
>> @@ -2225,7 +2225,7 @@ static int kfd_create_vcrat_image_gpu(void *pcrat_image,
>>  	 */
>>  	if (kdev->hive_id) {
>>  		for (nid = 0; nid < proximity_domain; ++nid) {
>> -			peer_dev = kfd_topology_device_by_proximity_domain(nid);
>> +			peer_dev = kfd_topology_device_by_proximity_domain_no_lock(nid);
>>  			if (!peer_dev->gpu)
>>  				continue;
>>  			if (peer_dev->gpu->hive_id != kdev->hive_id)
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> index e1b7e6afa920..8a43def1f638 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> @@ -1016,6 +1016,8 @@ int kfd_topology_add_device(struct kfd_dev *gpu);
>>  int kfd_topology_remove_device(struct kfd_dev *gpu);
>>  struct kfd_topology_device *kfd_topology_device_by_proximity_domain(
>>  						uint32_t proximity_domain);
>> +struct kfd_topology_device *kfd_topology_device_by_proximity_domain_no_lock(
>> +						uint32_t proximity_domain);
>>  struct kfd_topology_device *kfd_topology_device_by_id(uint32_t gpu_id);
>>  struct kfd_dev *kfd_device_by_id(uint32_t gpu_id);
>>  struct kfd_dev *kfd_device_by_pci_dev(const struct pci_dev *pdev);
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>> index 3bdcae239bc0..874a273b81f7 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>> @@ -46,27 +46,38 @@ static struct list_head topology_device_list;
>>  static struct kfd_system_properties sys_props;
>>    static DECLARE_RWSEM(topology_lock);
>> -static atomic_t topology_crat_proximity_domain;
>> +static uint32_t topology_crat_proximity_domain;
>>  -struct kfd_topology_device *kfd_topology_device_by_proximity_domain(
>> +struct kfd_topology_device *kfd_topology_device_by_proximity_domain_no_lock(
>>  						uint32_t proximity_domain)
>>  {
>>  	struct kfd_topology_device *top_dev;
>>  	struct kfd_topology_device *device = NULL;
>>  -	down_read(&topology_lock);
>> -
>>  	list_for_each_entry(top_dev, &topology_device_list, list)
>>  		if (top_dev->proximity_domain == proximity_domain) {
>>  			device = top_dev;
>>  			break;
>>  		}
>>  +	return device;
>> +}
>> +
>> +struct kfd_topology_device *kfd_topology_device_by_proximity_domain(
>> +						uint32_t proximity_domain)
>> +{
>> +	struct kfd_topology_device *device = NULL;
>> +
>> +	down_read(&topology_lock);
>> +
>> +	device = kfd_topology_device_by_proximity_domain_no_lock(
>> +							proximity_domain);
>>  	up_read(&topology_lock);
>>    	return device;
>>  }
>>  +
>>  struct kfd_topology_device *kfd_topology_device_by_id(uint32_t gpu_id)
>>  {
>>  	struct kfd_topology_device *top_dev = NULL;
>> @@ -1060,7 +1071,7 @@ int kfd_topology_init(void)
>>  	down_write(&topology_lock);
>>  	kfd_topology_update_device_list(&temp_topology_device_list,
>>  					&topology_device_list);
>> -	atomic_set(&topology_crat_proximity_domain, sys_props.num_devices-1);
>> +	topology_crat_proximity_domain = sys_props.num_devices-1;
>>  	ret = kfd_topology_update_sysfs();
>>  	up_write(&topology_lock);
>>  @@ -1295,8 +1306,6 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>>    	pr_debug("Adding new GPU (ID: 0x%x) to topology\n", gpu_id);
>>  -	proximity_domain = atomic_inc_return(&topology_crat_proximity_domain);
>> -
>>  	/* Include the CPU in xGMI hive if xGMI connected by assigning it the hive ID. */
>>  	if (gpu->hive_id && gpu->adev->gmc.xgmi.connected_to_cpu) {
>>  		struct kfd_topology_device *top_dev;
>> @@ -1321,12 +1330,16 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>>  	 */
>>  	dev = kfd_assign_gpu(gpu);
>>  	if (!dev) {
>> +		down_write(&topology_lock);
>> +		proximity_domain = ++topology_crat_proximity_domain;
>> +
>>  		res = kfd_create_crat_image_virtual(&crat_image, &image_size,
>>  						    COMPUTE_UNIT_GPU, gpu,
>>  						    proximity_domain);
>>  		if (res) {
>>  			pr_err("Error creating VCRAT for GPU (ID: 0x%x)\n",
>>  			       gpu_id);
>> +			topology_crat_proximity_domain--;
>>  			return res;
>>  		}
>>  		res = kfd_parse_crat_table(crat_image,
>> @@ -1335,10 +1348,10 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>>  		if (res) {
>>  			pr_err("Error parsing VCRAT for GPU (ID: 0x%x)\n",
>>  			       gpu_id);
>> +			topology_crat_proximity_domain--;
>>  			goto err;
>>  		}
>>  -		down_write(&topology_lock);
>>  		kfd_topology_update_device_list(&temp_topology_device_list,
>>  			&topology_device_list);
>>  @@ -1485,25 +1498,73 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>>  	return res;
>>  }
>>  +static void kfd_topology_update_io_links(int proximity_domain)
>> +{
>> +	struct kfd_topology_device *dev;
>> +	struct kfd_iolink_properties *iolink, *p2plink, *tmp;
>> +	/*
>> +	 * The topology list currently is arranged in increasing
>> +	 * order of proximity domain.
>> +	 *
>> +	 * Two things need to be done when a device is removed:
>> +	 * 1. All the IO links to this device need to be
>> +	 *    removed.
>> +	 * 2. All nodes after the current device node need to move
>> +	 *    up once this device node is removed from the topology
>> +	 *    list. As a result, the proximity domain values for
>> +	 *    all nodes after the node being deleted reduce by 1.
>> +	 *    This would also cause the proximity domain values for
>> +	 *    io links to be updated based on new proximity
>> +	 *    domain values.
>> +	 */
>> +	list_for_each_entry(dev, &topology_device_list, list) {
>> +		if (dev->proximity_domain > proximity_domain)
>> +			dev->proximity_domain--;
>> +
>> +		list_for_each_entry_safe(iolink, tmp, &dev->io_link_props, list) {
>> +			/*
>> +			 * If there is an io link to the dev being deleted
>> +			 * then remove that IO link also.
>> +			 */
>> +			if (iolink->node_to == proximity_domain) {
>> +				list_del(&iolink->list);
>> +				dev->io_link_count--;
>> +				dev->node_props.io_links_count--;
>> +			} else if (iolink->node_from > proximity_domain) {
>> +				iolink->node_from--;
>> +			} else if (iolink->node_to > proximity_domain) {
>> +				iolink->node_to--;
>> +			}
>> +		}
>> +
>> +	}
>> +}
>> +
>>  int kfd_topology_remove_device(struct kfd_dev *gpu)
>>  {
>>  	struct kfd_topology_device *dev, *tmp;
>>  	uint32_t gpu_id;
>>  	int res = -ENODEV;
>> +	int i = 0;
>>    	down_write(&topology_lock);
>>  -	list_for_each_entry_safe(dev, tmp, &topology_device_list, list)
>> +	list_for_each_entry_safe(dev, tmp, &topology_device_list, list) {
>>  		if (dev->gpu == gpu) {
>>  			gpu_id = dev->gpu_id;
>>  			kfd_remove_sysfs_node_entry(dev);
>>  			kfd_release_topology_device(dev);
>>  			sys_props.num_devices--;
>> +			kfd_topology_update_io_links(i);
>> +			topology_crat_proximity_domain = sys_props.num_devices-1;
>> +			sys_props.generation_count++;
>>  			res = 0;
>>  			if (kfd_topology_update_sysfs() < 0)
>>  				kfd_topology_release_sysfs();
>>  			break;
>>  		}
>> +		i++;
>> +	}
>>    	up_write(&topology_lock);
>>  





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

  Powered by Linux