linux-kernel - Re: [PATCHv3, RFC 31/34] thp: initial implementation of do_huge_linear

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51701D5E.80802@sr71.net>
Date:	Thu, 18 Apr 2013 09:20:46 -0700
From:	Dave Hansen <dave@...1.net>
To:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
CC:	Andrea Arcangeli <aarcange@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Hugh Dickins <hughd@...gle.com>,
	Wu Fengguang <fengguang.wu@...el.com>, Jan Kara <jack@...e.cz>,
	Mel Gorman <mgorman@...e.de>, linux-mm@...ck.org,
	Andi Kleen <ak@...ux.intel.com>,
	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	"Kirill A. Shutemov" <kirill@...temov.name>,
	Hillf Danton <dhillf@...il.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3, RFC 31/34] thp: initial implementation of do_huge_linear_fault()

On 04/18/2013 09:09 AM, Kirill A. Shutemov wrote:
> Dave Hansen wrote:
>> On 04/17/2013 07:38 AM, Kirill A. Shutemov wrote:
>> Are you still sure you can't do _any_ better than a verbatim copy of 129
>> lines?
> 
> It seems I was too lazy. Shame on me. :(
> Here's consolidated version. Only build tested. Does it look better?

Yeah, it's definitely a step in the right direction.  There rae
definitely some bugs in there like:

+	unsigned long haddr = address & PAGE_MASK;

I do think some of this refactoring stuff

> -				unlock_page(page);
> -				vmf.flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
> -				tmp = vma->vm_ops->page_mkwrite(vma, &vmf);
> -				if (unlikely(tmp &
> -					  (VM_FAULT_ERROR | VM_FAULT_NOPAGE))) {
> -					ret = tmp;
> +			unlock_page(page);
> +			vmf.flags = FAULT_FLAG_WRITE | FAULT_FLAG_MKWRITE;
> +			tmp = vma->vm_ops->page_mkwrite(vma, &vmf);
> +			if (unlikely(tmp &
> +					(VM_FAULT_ERROR | VM_FAULT_NOPAGE))) {
> +				ret = tmp;
> +				goto unwritable_page;
> +			}

could probably be a separate patch and would make what's going on more
clear, but it's passable the way it is.  When it is done this way it's
hard sometimes reading the diff to realize if you are adding code or
just moving it around.

This stuff:

>  		if (set_page_dirty(dirty_page))
> -			dirtied = 1;
> +			dirtied = true;

needs to go in another patch for sure.

One thing I *REALLY* like about doing patches this way is that things
like this start to pop out:

> -	ret = vma->vm_ops->fault(vma, &vmf);
> +	if (try_huge_pages) {
> +		pgtable = pte_alloc_one(mm, haddr);
> +		if (unlikely(!pgtable)) {
> +			ret = VM_FAULT_OOM;
> +			goto uncharge_out;
> +		}
> +		ret = vma->vm_ops->huge_fault(vma, &vmf);
> +	} else
> +		ret = vma->vm_ops->fault(vma, &vmf);

The ->fault is (or can be) essentially per filesystem, and we're going
to be adding support per-filesystem.  any reason we can't just handle
this inside the ->fault code and avoid adding huge_fault altogether?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/