On Tue, Mar 17, 2020 at 04:14:31PM -0700, Ralph Campbell wrote:
On 3/17/20 5:59 AM, Christoph Hellwig wrote:
On Tue, Mar 17, 2020 at 09:47:55AM -0300, Jason Gunthorpe wrote:
I've been using v7 of Ralph's tester and it is working well - it has DEVICE_PRIVATE support so I think it can test this flow too. Ralph are you able?
This hunk seems trivial enough to me, can we include it now?
I can send a separate patch for it once the tester covers it. I don't want to add it to the original patch as it is a significant behavior change compared to the existing code.
Attached is an updated version of my HMM tests based on linux-5.6.0-rc6. I ran this OK with Jason's 8+1 HMM patches, Christoph's 1-5 misc HMM clean ups, and Christoph's 1-4 device private page changes applied.
I'd like to get this to mergable, it looks pretty good now, but I have no idea about selftests - and I'm struggling to even compile the tools dir
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 69def4a9df00..4d22ce7879a7 100644 +++ b/lib/Kconfig.debug @@ -2162,6 +2162,18 @@ config TEST_MEMINIT
If unsure, say N.+config TEST_HMM
- tristate "Test HMM (Heterogeneous Memory Management)"
- depends on DEVICE_PRIVATE
- select HMM_MIRROR
select MMU_NOTIFIER
extra spaces
In general I wonder if it even makes sense that DEVICE_PRIVATE is user selectable?
+static int dmirror_fops_open(struct inode *inode, struct file *filp) +{
- struct cdev *cdev = inode->i_cdev;
- struct dmirror *dmirror;
- int ret;
- /* Mirror this process address space */
- dmirror = kzalloc(sizeof(*dmirror), GFP_KERNEL);
- if (dmirror == NULL)
return -ENOMEM;- dmirror->mdevice = container_of(cdev, struct dmirror_device, cdevice);
- mutex_init(&dmirror->mutex);
- xa_init(&dmirror->pt);
- ret = mmu_interval_notifier_insert(&dmirror->notifier, current->mm,
0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);- if (ret) {
kfree(dmirror);return ret;- }
- /* Pairs with the mmdrop() in dmirror_fops_release(). */
- mmgrab(current->mm);
- dmirror->mm = current->mm;
The notifier holds a mmgrab, no need for another one
- /* Only the first open registers the address space. */
- filp->private_data = dmirror;
Not sure what this comment means
+static inline struct dmirror_device *dmirror_page_to_device(struct page *page)
+{
- struct dmirror_chunk *devmem;
- devmem = container_of(page->pgmap, struct dmirror_chunk, pagemap);
- return devmem->mdevice;
+}
extra devmem var is not really needed
+static bool dmirror_device_is_mine(struct dmirror_device *mdevice,
struct page *page)+{
- if (!is_zone_device_page(page))
return false;- return page->pgmap->ops == &dmirror_devmem_ops &&
dmirror_page_to_device(page) == mdevice;+}
Use new owner stuff, right? Actually this is redunant now, the check should be just WARN_ON pageowner != self owner
+static int dmirror_do_fault(struct dmirror *dmirror, struct hmm_range *range) +{
- uint64_t *pfns = range->pfns;
- unsigned long pfn;
- for (pfn = (range->start >> PAGE_SHIFT);
pfn < (range->end >> PAGE_SHIFT);pfn++, pfns++) {struct page *page;void *entry;/** HMM_PFN_ERROR is returned if it is accessing invalid memory* either because of memory error (hardware detected memory* corruption) or more likely because of truncate on mmap* file.*/if (*pfns == range->values[HMM_PFN_ERROR])return -EFAULT;
Unless that snapshot is use hmm_range_fault() never returns success and sets PFN_ERROR, so this should be a WARN_ON
if (!(*pfns & range->flags[HMM_PFN_VALID]))return -EFAULT;
Same with valid.
page = hmm_device_entry_to_page(range, *pfns);/* We asked for pages to be populated but check anyway. */if (!page)return -EFAULT;
WARN_ON
if (is_zone_device_page(page)) {/** TODO: need a way to ask HMM to fault foreign zone* device private pages.*/if (!dmirror_device_is_mine(dmirror->mdevice, page))continue;
Actually re
+static bool dmirror_interval_invalidate(struct mmu_interval_notifier *mni,
const struct mmu_notifier_range *range,unsigned long cur_seq)+{
- struct dmirror *dmirror = container_of(mni, struct dmirror, notifier);
- struct mm_struct *mm = dmirror->mm;
- /*
* If the process doesn't exist, we don't need to invalidate the* device page table since the address space will be torn down.*/- if (!mmget_not_zero(mm))
return true;
Why? Don't the notifiers provide for this already.
mmget_not_zero() is required before calling hmm_range_fault() though
+static int dmirror_fault(struct dmirror *dmirror, unsigned long start,
unsigned long end, bool write)+{
- struct mm_struct *mm = dmirror->mm;
- unsigned long addr;
- uint64_t pfns[64];
- struct hmm_range range = {
.notifier = &dmirror->notifier,.pfns = pfns,.flags = dmirror_hmm_flags,.values = dmirror_hmm_values,.pfn_shift = DPT_SHIFT,.pfn_flags_mask = ~(dmirror_hmm_flags[HMM_PFN_VALID] |dmirror_hmm_flags[HMM_PFN_WRITE]),.default_flags = dmirror_hmm_flags[HMM_PFN_VALID] |(write ? dmirror_hmm_flags[HMM_PFN_WRITE] : 0),.dev_private_owner = dmirror->mdevice,- };
- int ret = 0;
- /* Since the mm is for the mirrored process, get a reference first. */
- if (!mmget_not_zero(mm))
return 0;
Right
- for (addr = start; addr < end; addr = range.end) {
range.start = addr;range.end = min(addr + (ARRAY_SIZE(pfns) << PAGE_SHIFT), end);ret = dmirror_range_fault(dmirror, &range);if (ret)break;- }
- mmput(mm);
- return ret;
+}
+static int dmirror_do_read(struct dmirror *dmirror, unsigned long start,
unsigned long end, struct dmirror_bounce *bounce)+{
- unsigned long pfn;
- void *ptr;
- ptr = bounce->ptr + ((start - bounce->addr) & PAGE_MASK);
- for (pfn = start >> PAGE_SHIFT; pfn < (end >> PAGE_SHIFT); pfn++) {
void *entry;struct page *page;void *tmp;entry = xa_load(&dmirror->pt, pfn);page = xa_untag_pointer(entry);if (!page)return -ENOENT;tmp = kmap(page);memcpy(ptr, tmp, PAGE_SIZE);kunmap(page);ptr += PAGE_SIZE;bounce->cpages++;- }
- return 0;
+}
+static int dmirror_read(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd) +{
- struct dmirror_bounce bounce;
- unsigned long start, end;
- unsigned long size = cmd->npages << PAGE_SHIFT;
- int ret;
- start = cmd->addr;
- end = start + size;
- if (end < start)
return -EINVAL;- ret = dmirror_bounce_init(&bounce, start, size);
- if (ret)
return ret;+again:
- mutex_lock(&dmirror->mutex);
- ret = dmirror_do_read(dmirror, start, end, &bounce);
- mutex_unlock(&dmirror->mutex);
- if (ret == 0)
ret = copy_to_user((void __user *)cmd->ptr, bounce.ptr,bounce.size);
Use u64_to_user_ptr() instead of the cast
+static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd) +{
- struct dmirror_bounce bounce;
- unsigned long start, end;
- unsigned long size = cmd->npages << PAGE_SHIFT;
- int ret;
- start = cmd->addr;
- end = start + size;
- if (end < start)
return -EINVAL;- ret = dmirror_bounce_init(&bounce, start, size);
- if (ret)
return ret;- ret = copy_from_user(bounce.ptr, (void __user *)cmd->ptr,
bounce.size);
ditto
- if (ret)
return ret;+again:
- mutex_lock(&dmirror->mutex);
- ret = dmirror_do_write(dmirror, start, end, &bounce);
- mutex_unlock(&dmirror->mutex);
- if (ret == -ENOENT) {
start = cmd->addr + (bounce.cpages << PAGE_SHIFT);ret = dmirror_fault(dmirror, start, end, true);if (ret == 0) {cmd->faults++;goto again;
Use a loop instead of goto?
Also I get this:
lib/test_hmm.c: In function ‘dmirror_devmem_fault_alloc_and_copy’: lib/test_hmm.c:1041:25: warning: unused variable ‘vma’ [-Wunused-variable] 1041 | struct vm_area_struct *vma = args->vma;
But this is a kernel bug, due to alloc_page_vma being a #define not a static inline and me having CONFIG_NUMA off in this .config
Jason