Re: [RFC 1/8] Introduce new system call mvolatile

[Minchan Kim <minchan@kernel.org>]

On Wed, Jan 16, 2013 at 05:48:37PM -0800, John Stultz wrote:
> On 01/02/2013 08:27 PM, Minchan Kim wrote:
> >This patch adds new system call m[no]volatile.
> >If someone asks is_volatile system call, it could be added, too.
> 
> So some nits below from my initial playing around with this patchset.
> 
> >+/*
> >+ * Return -EINVAL if range doesn't include a right vma at all.
> >+ * Return -ENOMEM with interrupting range opeartion if memory is not enough to
> >+ * merge/split vmas.
> >+ * Return 0 if range consists of only proper vmas.
> >+ * Return 1 if part of range includes inavlid area(ex, hole/huge/ksm/mlock/
> >+ * special area)
> >+ */
> >+SYSCALL_DEFINE2(mvolatile, unsigned long, start, size_t, len)
> >+{
> >+	unsigned long end, tmp;
> >+	struct vm_area_struct *vma, *prev;
> >+	bool invalid = false;
> >+	int error = -EINVAL;
> >+
> >+	down_write(&current->mm->mmap_sem);
> >+	if (start & ~PAGE_MASK)
> >+		goto out;
> >+
> >+	len &= PAGE_MASK;
> >+	if (!len)
> >+		goto out;
> >+
> >+	end = start + len;
> >+	if (end < start)
> >+		goto out;
> >+
> >+	vma = find_vma_prev(current->mm, start, &prev);
> >+	if (!vma)
> >+		goto out;
> >+
> >+	if (start > vma->vm_start)
> >+		prev = vma;
> >+
> >+	for (;;) {
> >+		/* Here start < (end|vma->vm_end). */
> >+		if (start < vma->vm_start) {
> >+			start = vma->vm_start;
> >+			if (start >= end)
> >+				goto out;
> >+			invalid = true;
> >+		}
> >+
> >+		/* Here vma->vm_start <= start < (end|vma->vm_end) */
> >+		tmp = vma->vm_end;
> >+		if (end < tmp)
> >+			tmp = end;
> >+
> >+		/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> >+		error = do_mvolatile(vma, &prev, start, tmp);
> >+		if (error == -ENOMEM) {
> >+			up_write(&current->mm->mmap_sem);
> >+			return error;
> >+		}
> >+		if (error == -EINVAL)
> >+			invalid = true;
> >+		else
> >+			error = 0;
> >+		start = tmp;
> >+		if (prev && start < prev->vm_end)
> >+			start = prev->vm_end;
> >+		if (start >= end)
> >+			break;
> >+
> >+		vma = prev->vm_next;
> >+		if (!vma)
> >+			break;
> >+	}
> >+out:
> >+	up_write(&current->mm->mmap_sem);
> >+	return invalid ? 1 : 0;
> >+}
> 
> The error logic here is really strange. If any of the early error
> cases are triggered (ie: (start & ~PAGE_MASK), etc), then we jump to
> out and return 0 (instead of EINVAL). I don't think that's what you
> intended.

Need fixing.

> 
> 
> >+/*
> >+ * Return -ENOMEM with interrupting range opeartion if memory is not enough
> >+ * to merge/split vmas.
> >+ * Return 1 if part of range includes purged's one, otherwise, return 0
> >+ */
> >+SYSCALL_DEFINE2(mnovolatile, unsigned long, start, size_t, len)
> >+{
> >+	unsigned long end, tmp;
> >+	struct vm_area_struct *vma, *prev;
> >+	int ret, error = -EINVAL;
> >+	bool is_purged = false;
> >+
> >+	down_write(&current->mm->mmap_sem);
> >+	if (start & ~PAGE_MASK)
> >+		goto out;
> >+
> >+	len &= PAGE_MASK;
> >+	if (!len)
> >+		goto out;
> >+
> >+	end = start + len;
> >+	if (end < start)
> >+		goto out;
> >+
> >+	vma = find_vma_prev(current->mm, start, &prev);
> >+	if (!vma)
> >+		goto out;
> >+
> >+	if (start > vma->vm_start)
> >+		prev = vma;
> >+
> >+	for (;;) {
> >+		/* Here start < (end|vma->vm_end). */
> >+		if (start < vma->vm_start) {
> >+			start = vma->vm_start;
> >+			if (start >= end)
> >+				goto out;
> >+		}
> >+
> >+		/* Here vma->vm_start <= start < (end|vma->vm_end) */
> >+		tmp = vma->vm_end;
> >+		if (end < tmp)
> >+			tmp = end;
> >+
> >+		/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> >+		error = do_mnovolatile(vma, &prev, start, tmp, &is_purged);
> >+		if (error) {
> >+			WARN_ON(error != -ENOMEM);
> >+			goto out;
> >+		}
> >+		start = tmp;
> >+		if (prev && start < prev->vm_end)
> >+			start = prev->vm_end;
> >+		if (start >= end)
> >+			break;
> >+
> >+		vma = prev->vm_next;
> >+		if (!vma)
> >+			break;
> >+	}
> 
> I'm still not sure how this logic improves over the madvise case. If
> we catch an error mid-way through setting a series of vmas to
> non-volatile, we end up exiting and losing state (ie: if only the
> first vma was purged, but half way through 10 vmas we get a ENOMEM
> error. So the first vma is now non-volatile, but we do not return
> the purged flag ).

Right. 

> 
> If we're going to have a new syscall for this (which I'm not sure is
> the right approach), we should make use of multiple arguments so we
> can return if data was purged, even if we hit an error midway).

It would be easier method to achieve our goal than below suggestion
in case of VMA-basd approach because it's hard to expect how many
we need vmas with atomically.

Will do it in next version.

> 
> Alternatively, if we can find a way to allocate any necessary memory
> before we do any vma volatility state changes, then we can return
> ENOMEM then and be confident we won't end up with failed partial
> state change (this is the approach I used in my fallocate-volatile
> patches).

Thanks for the review, John.

> 
> thanks
> -john
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>