All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>
To: Alistair Popple <apopple@nvidia.com>,
	akpm@linux-foundation.org, Felix.Kuehling@amd.com,
	linux-mm@kvack.org, rcampbell@nvidia.com,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org
Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	hch@lst.de, jgg@nvidia.com, jglisse@redhat.com
Subject: Re: [PATCH v2 09/12] lib: test_hmm add module param for zone device type
Date: Mon, 20 Sep 2021 15:05:30 -0500	[thread overview]
Message-ID: <729f833c-e880-96ce-5f49-2d72a93faa21@amd.com> (raw)
In-Reply-To: <2139200.KYAmxeAneQ@nvdebian>


On 9/20/2021 3:53 AM, Alistair Popple wrote:
> On Tuesday, 14 September 2021 2:16:01 AM AEST Alex Sierra wrote:
>> In order to configure device public in test_hmm, two module parameters
>> should be passed, which correspond to the SP start address of each
>> device (2) spm_addr_dev0 & spm_addr_dev1. If no parameters are passed,
>> private device type is configured.
> It's a pity that testing this seems to require some amount of special setup to
> test. Is there a way this could be made to work on a more standard setup
> similar to how DEVICE_PRIVATE is tested?
Hi Alistair
We tried to do it as simpler as possible. Unfortunately, there are two main
requirements to register dev memory as DEVICE_PUBLIC type. This memory must
NOT be accessed by any memory allocator (SLAB, SLOB, SLUB) plus, it has 
to be
CPU coherently accessed.  We also want to avoid aliasing the same PFNs for
different page types (regular system memory and DEVICE_PUBLIC). So we don't
want the reserved memory to be part of the kernel's memory map before we 
call
memremap_pages. A transparent way of doing it, without any special HW, was
setting a portion of system memory as SPM (Special purpose memory). And use
this as our “device fake” memory.

Regards,
Alex Sierra

>
>> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
>> ---
>> v5:
>> Remove devmem->pagemap.type = MEMORY_DEVICE_PRIVATE at
>> dmirror_allocate_chunk that was forcing to configure pagemap.type
>> to MEMORY_DEVICE_PRIVATE
>>
>> v6:
>> Check for null pointers for resource and memremap references
>> at dmirror_allocate_chunk
>>
>> v7:
>> Due to patch dropped from these patch series "kernel: resource:
>> lookup_resource as exported symbol", lookup_resource was not longer a
>> callable function. This was used in public device configuration, to
>> get start and end addresses, to create pgmap->range struct. This
>> information is now taken directly from the spm_addr_devX parameters and
>> the fixed size DEVMEM_CHUNK_SIZE.
>> ---
>>   lib/test_hmm.c      | 66 +++++++++++++++++++++++++++++++--------------
>>   lib/test_hmm_uapi.h |  1 +
>>   2 files changed, 47 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
>> index 3cd91ca31dd7..ef27e355738a 100644
>> --- a/lib/test_hmm.c
>> +++ b/lib/test_hmm.c
>> @@ -33,6 +33,16 @@
>>   #define DEVMEM_CHUNK_SIZE		(256 * 1024 * 1024U)
>>   #define DEVMEM_CHUNKS_RESERVE		16
>>   
>> +static unsigned long spm_addr_dev0;
>> +module_param(spm_addr_dev0, long, 0644);
>> +MODULE_PARM_DESC(spm_addr_dev0,
>> +		"Specify start address for SPM (special purpose memory) used for device 0. By setting this Generic device type will be used. Make sure spm_addr_dev1 is set too");
>> +
>> +static unsigned long spm_addr_dev1;
>> +module_param(spm_addr_dev1, long, 0644);
>> +MODULE_PARM_DESC(spm_addr_dev1,
>> +		"Specify start address for SPM (special purpose memory) used for device 1. By setting this Generic device type will be used. Make sure spm_addr_dev0 is set too");
>> +
>>   static const struct dev_pagemap_ops dmirror_devmem_ops;
>>   static const struct mmu_interval_notifier_ops dmirror_min_ops;
>>   static dev_t dmirror_dev;
>> @@ -450,11 +460,11 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
>>   	return ret;
>>   }
>>   
>> -static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
>> +static int dmirror_allocate_chunk(struct dmirror_device *mdevice,
>>   				   struct page **ppage)
>>   {
>>   	struct dmirror_chunk *devmem;
>> -	struct resource *res;
>> +	struct resource *res = NULL;
>>   	unsigned long pfn;
>>   	unsigned long pfn_first;
>>   	unsigned long pfn_last;
>> @@ -462,17 +472,29 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
>>   
>>   	devmem = kzalloc(sizeof(*devmem), GFP_KERNEL);
>>   	if (!devmem)
>> -		return false;
>> +		return -ENOMEM;
>>   
>> -	res = request_free_mem_region(&iomem_resource, DEVMEM_CHUNK_SIZE,
>> -				      "hmm_dmirror");
>> -	if (IS_ERR(res))
>> -		goto err_devmem;
>> +	if (!spm_addr_dev0 && !spm_addr_dev1) {
>> +		res = request_free_mem_region(&iomem_resource, DEVMEM_CHUNK_SIZE,
>> +					      "hmm_dmirror");
>> +		if (IS_ERR_OR_NULL(res))
>> +			goto err_devmem;
>> +		devmem->pagemap.range.start = res->start;
>> +		devmem->pagemap.range.end = res->end;
>> +		devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
>> +		mdevice->zone_device_type = HMM_DMIRROR_MEMORY_DEVICE_PRIVATE;
>> +	} else if (spm_addr_dev0 && spm_addr_dev1) {
>> +		devmem->pagemap.range.start = MINOR(mdevice->cdevice.dev) ?
>> +							spm_addr_dev0 :
>> +							spm_addr_dev1;
>> +		devmem->pagemap.range.end = devmem->pagemap.range.start +
>> +					    DEVMEM_CHUNK_SIZE - 1;
>> +		devmem->pagemap.type = MEMORY_DEVICE_PUBLIC;
>> +		mdevice->zone_device_type = HMM_DMIRROR_MEMORY_DEVICE_PUBLIC;
>> +	} else {
>> +		pr_err("Both spm_addr_dev parameters should be set\n");
>> +	}
>>   
>> -	mdevice->zone_device_type = HMM_DMIRROR_MEMORY_DEVICE_PRIVATE;
>> -	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
>> -	devmem->pagemap.range.start = res->start;
>> -	devmem->pagemap.range.end = res->end;
>>   	devmem->pagemap.nr_range = 1;
>>   	devmem->pagemap.ops = &dmirror_devmem_ops;
>>   	devmem->pagemap.owner = mdevice;
>> @@ -493,10 +515,14 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
>>   		mdevice->devmem_capacity = new_capacity;
>>   		mdevice->devmem_chunks = new_chunks;
>>   	}
>> -
>>   	ptr = memremap_pages(&devmem->pagemap, numa_node_id());
>> -	if (IS_ERR(ptr))
>> +	if (IS_ERR_OR_NULL(ptr)) {
>> +		if (ptr)
>> +			ret = PTR_ERR(ptr);
>> +		else
>> +			ret = -EFAULT;
>>   		goto err_release;
>> +	}
>>   
>>   	devmem->mdevice = mdevice;
>>   	pfn_first = devmem->pagemap.range.start >> PAGE_SHIFT;
>> @@ -529,7 +555,8 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
>>   
>>   err_release:
>>   	mutex_unlock(&mdevice->devmem_lock);
>> -	release_mem_region(devmem->pagemap.range.start, range_len(&devmem->pagemap.range));
>> +	if (res)
>> +		release_mem_region(devmem->pagemap.range.start, range_len(&devmem->pagemap.range));
>>   err_devmem:
>>   	kfree(devmem);
>>   
>> @@ -1097,10 +1124,8 @@ static int dmirror_device_init(struct dmirror_device *mdevice, int id)
>>   	if (ret)
>>   		return ret;
>>   
>> -	/* Build a list of free ZONE_DEVICE private struct pages */
>> -	dmirror_allocate_chunk(mdevice, NULL);
>> -
>> -	return 0;
>> +	/* Build a list of free ZONE_DEVICE struct pages */
>> +	return dmirror_allocate_chunk(mdevice, NULL);
>>   }
>>   
>>   static void dmirror_device_remove(struct dmirror_device *mdevice)
>> @@ -1113,8 +1138,9 @@ static void dmirror_device_remove(struct dmirror_device *mdevice)
>>   				mdevice->devmem_chunks[i];
>>   
>>   			memunmap_pages(&devmem->pagemap);
>> -			release_mem_region(devmem->pagemap.range.start,
>> -					   range_len(&devmem->pagemap.range));
>> +			if (devmem->pagemap.type == MEMORY_DEVICE_PRIVATE)
>> +				release_mem_region(devmem->pagemap.range.start,
>> +						   range_len(&devmem->pagemap.range));
>>   			kfree(devmem);
>>   		}
>>   		kfree(mdevice->devmem_chunks);
>> diff --git a/lib/test_hmm_uapi.h b/lib/test_hmm_uapi.h
>> index ee88701793d5..00259d994410 100644
>> --- a/lib/test_hmm_uapi.h
>> +++ b/lib/test_hmm_uapi.h
>> @@ -65,6 +65,7 @@ enum {
>>   enum {
>>   	/* 0 is reserved to catch uninitialized type fields */
>>   	HMM_DMIRROR_MEMORY_DEVICE_PRIVATE = 1,
>> +	HMM_DMIRROR_MEMORY_DEVICE_PUBLIC,
>>   };
>>   
>>   #endif /* _LIB_TEST_HMM_UAPI_H */
>>
>
>
>

  reply	other threads:[~2021-09-20 20:07 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-13 16:15 [PATCH v2 00/12] MEMORY_DEVICE_PUBLIC for CPU-accessible coherent device memory Alex Sierra
2021-09-13 16:15 ` [PATCH v2 01/12] ext4/xfs: add page refcount helper Alex Sierra
2021-09-13 16:15 ` [PATCH v2 02/12] mm: remove extra ZONE_DEVICE struct page refcount Alex Sierra
2021-09-15  5:32   ` Ralph Campbell
2021-09-15  5:32     ` Ralph Campbell
2021-09-13 16:15 ` [PATCH v2 03/12] mm: add zone device public type memory support Alex Sierra
2021-09-13 16:15 ` [PATCH v2 04/12] mm: add device public vma selection for memory migration Alex Sierra
2021-09-13 16:15 ` [PATCH v2 05/12] drm/amdkfd: ref count init for device pages Alex Sierra
2021-09-13 16:15 ` [PATCH v2 06/12] drm/amdkfd: add SPM support for SVM Alex Sierra
2021-09-13 16:15 ` [PATCH v2 07/12] drm/amdkfd: public type as sys mem on migration to ram Alex Sierra
2021-09-13 16:16 ` [PATCH v2 08/12] lib: test_hmm add ioctl to get zone device type Alex Sierra
2021-09-13 16:16 ` [PATCH v2 09/12] lib: test_hmm add module param for " Alex Sierra
2021-09-20  8:53   ` Alistair Popple
2021-09-20 20:05     ` Sierra Guiza, Alejandro (Alex) [this message]
2021-09-21  5:14       ` Alistair Popple
2021-09-23 15:52         ` Sierra Guiza, Alejandro (Alex)
2021-10-01  0:28           ` Alistair Popple
2021-09-13 16:16 ` [PATCH v2 10/12] lib: add support for device public type in test_hmm Alex Sierra
2021-10-01  1:32   ` Alistair Popple
2021-09-13 16:16 ` [PATCH v2 11/12] tools: update hmm-test to support device public type Alex Sierra
2021-09-13 16:16 ` [PATCH v2 12/12] tools: update test_hmm script to support SP config Alex Sierra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=729f833c-e880-96ce-5f49-2d72a93faa21@amd.com \
    --to=alex.sierra@amd.com \
    --cc=Felix.Kuehling@amd.com \
    --cc=akpm@linux-foundation.org \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=apopple@nvidia.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hch@lst.de \
    --cc=jgg@nvidia.com \
    --cc=jglisse@redhat.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=rcampbell@nvidia.com \
    --subject='Re: [PATCH v2 09/12] lib: test_hmm add module param for zone device type' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.