Re: [RFC PATCH v3 1/2] mempinfd: Add new syscall to provide memory pin

On 2021/2/8 5:34, Matthew Wilcox wrote:

On Sun, Feb 07, 2021 at 04:18:03PM +0800, Zhou Wang wrote:

quoted

SVA(share virtual address) offers a way for device to share process virtual
address space safely, which makes more convenient for user space device
driver coding. However, IO page faults may happen when doing DMA
operations. As the latency of IO page fault is relatively big, DMA
performance will be affected severely when there are IO page faults.

quoted

From a long term view, DMA performance will be not stable.

In high-performance I/O cases, accelerators might want to perform
I/O on a memory without IO page faults which can result in dramatically
increased latency. Current memory related APIs could not achieve this
requirement, e.g. mlock can only avoid memory to swap to backup device,
page migration can still trigger IO page fault.

Well ... we have two requirements.  The application wants to not take
page faults.  The system wants to move the application to a different
NUMA node in order to optimise overall performance.  Why should the
application's desires take precedence over the kernel's desires?  And why
should it be done this way rather than by the sysadmin using numactl to
lock the application to a particular node?

Just as Barry mentioned, there are other cases which could trigger IOPF.
Only numactl is not enough.

quoted

+struct mem_pin_container {
+	struct xarray array;
+	struct mutex lock;
+};

I don't understand what the lock actually protects.

This lock protects pin/unpin and record/remove.
 - pin pages and record them
 - unpin pages and remove them
should be exlusive.

quoted

+struct pin_pages {
+	unsigned long first;
+	unsigned long nr_pages;
+	struct page **pages;
+};

I don't think you need 'first', and I think you can embed the pages
array into this struct, removing one allocation.

'first' will be recorded and be used to unpin later. We use it
as an index to get pinned pages and do unpin operation.

quoted

+	xa_for_each(&priv->array, idx, p) {
+		unpin_user_pages(p->pages, p->nr_pages);
+		xa_erase(&priv->array, p->first);
+		vfree(p->pages);
+		kfree(p);
+	}
+
+	mutex_destroy(&priv->lock);
+	xa_destroy(&priv->array);

If you just called xa_erase() on every element of the array, you don't need
to call xa_destroy().

OK.

quoted

+	if (!can_do_mlock())
+		return -EPERM;

You check for can_do_mlock(), but you don't account the pages to this
rlimit.

Here I just copied it from ib_umen_get and do_mlock. If needed, we can
add account for pages here.

quoted

+	first = (addr->addr & PAGE_MASK) >> PAGE_SHIFT;

You don't need to mask off the bits, the shift will remove them.

OK.

quoted

+	last = ((addr->addr + addr->size - 1) & PAGE_MASK) >> PAGE_SHIFT;

DIV_ROUND_UP()?

addr->size is input pin page size which is not same as PAGE_SIZE.
So seems we can not use this macro.

quoted

+	pages = vmalloc(nr_pages * sizeof(struct page *));

kvmalloc().  vmalloc() always allocates at least a page, so we want to
use kmalloc if the size is small.  Also, use array_size() -- I know this
can't overflow, but let's be clear

Yes, will use kvmalloc and array_size here.

quoted

+	ret = pin_user_pages_fast(addr->addr & PAGE_MASK, nr_pages,
+				  flags | FOLL_LONGTERM, pages);
+	if (ret != nr_pages) {
+		pr_err("mempinfd: Failed to pin page\n");

No.  You mustn't allow the user to be able to generate messages to syslog,
just by passing garbage to a syscall.

OK, will remove this.

quoted

+	ret = xa_insert(&priv->array, p->first, p, GFP_KERNEL);
+	if (ret)
+		goto unpin_pages;

Hmm.  So we can't pin two ranges which start at the same address, but we
can pin two overlapping ranges.  Is that OK?

The design here is only supporting pin a range with different start address.
If two overlapping ranges with different start address, we will not prevent
this. If this will not go wrong, let's make it simple firstly.

Best,
Zhou

.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel