All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alistair Popple <apopple@nvidia.com>
To: <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>,
	"Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>
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: Fri, 1 Oct 2021 10:28:36 +1000	[thread overview]
Message-ID: <5733707.7NG8XZRkRI@nvdebian> (raw)
In-Reply-To: <30468c52-62b6-1af8-07dd-e9be766b6b77@amd.com>

On Friday, 24 September 2021 1:52:47 AM AEST Sierra Guiza, Alejandro (Alex) wrote:
> 
> On 9/21/2021 12:14 AM, Alistair Popple wrote:
> > On Tuesday, 21 September 2021 6:05:30 AM AEST Sierra Guiza, Alejandro (Alex) wrote:
> >> 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.
> > Ok, I think it's great that we can test this without special HW but the boot
> > time configuration is still a bit annoying. Would it be possible to allocate
> > memory fitting the above requirements by hot unplugging it with something like
> > offline_and_remove_memory()?
> > I also don't see why the DEVICE_PRIVATE and DEVICE_PUBLIC testing should be
> > mutually exclusive - why can't we test both without reloading the module?
> You could do both DEVICE_PRIVATE and DEVICE_PUBLIC tests by running the 
> test_hmm_sh
> script twice, just passing the proper parameters. Even when you booted 
> with fake EFI SP
> regions. If spm_address_dev0/1 parameters are passed, the module gets 
> configured with
> DEVICE_PUBLIC type. Otherwise DEVICE_PRIVATE type is set. Technically 
> the only
> complication in testing DEVICE_PUBLIC is the fake SPM boot parameter.

Or you could just have the test specify what sort of memory it wants to use
(DEVICE_PRIVATE or DEVICE_GENERIC). That seems preferable to requiring a module
reload. A module reload also makes it impossible to test interactions between
DEVICE_PRIVATE and DEVICE_GENERIC memory.

 - Alistair

> Alex Sierra
> >
> >   - Alistair
> >
> >> 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-10-01  0:28 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)
2021-09-21  5:14       ` Alistair Popple
2021-09-23 15:52         ` Sierra Guiza, Alejandro (Alex)
2021-10-01  0:28           ` Alistair Popple [this message]
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=5733707.7NG8XZRkRI@nvdebian \
    --to=apopple@nvidia.com \
    --cc=Felix.Kuehling@amd.com \
    --cc=akpm@linux-foundation.org \
    --cc=alex.sierra@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --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 \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.