linux-kernel - Re: [PATCH 06/36] HMM: add HMM page table v2.

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: <alpine.DEB.2.00.1506301946190.31456@mdh-linux64-2.nvidia.com>
Date:	Tue, 30 Jun 2015 19:51:12 -0700
From:	Mark Hairgrove <mhairgrove@...dia.com>
To:	Jerome Glisse <j.glisse@...il.com>
CC:	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"joro@...tes.org" <joro@...tes.org>, Mel Gorman <mgorman@...e.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Johannes Weiner <jweiner@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Dave Airlie <airlied@...hat.com>,
	Brendan Conoboy <blc@...hat.com>,
	Joe Donohue <jdonohue@...hat.com>,
	Duncan Poole <dpoole@...dia.com>,
	Sherry Cheung <SCheung@...dia.com>,
	Subhash Gutti <sgutti@...dia.com>,
	John Hubbard <jhubbard@...dia.com>,
	Lucien Dunning <ldunning@...dia.com>,
	Cameron Buschardt <cabuschardt@...dia.com>,
	Arvind Gopalakrishnan <arvindg@...dia.com>,
	Haggai Eran <haggaie@...lanox.com>,
	Shachar Raindel <raindel@...lanox.com>,
	Liran Liss <liranl@...lanox.com>,
	Roland Dreier <roland@...estorage.com>,
	Ben Sander <ben.sander@....com>,
	Greg Stoner <Greg.Stoner@....com>,
	John Bridgman <John.Bridgman@....com>,
	Michael Mantor <Michael.Mantor@....com>,
	Paul Blinzer <Paul.Blinzer@....com>,
	Laurent Morichetti <Laurent.Morichetti@....com>,
	Alexander Deucher <Alexander.Deucher@....com>,
	Oded Gabbay <Oded.Gabbay@....com>,
	Jérôme Glisse <jglisse@...hat.com>,
	Jatin Kumar <jakumar@...dia.com>
Subject: Re: [PATCH 06/36] HMM: add HMM page table v2.



On Mon, 29 Jun 2015, Jerome Glisse wrote:

> [...]
> 
> Iterator is what protect against concurrent freeing of the directory so it
> has to return to caller on directory boundary (for 64bits arch with 64bits
> pte it has return every 512 entries). Otherwise pt_iter_fini() would have
> to walk over the whole directory range again just to drop reference and this
> doesn't sound like a good idea.

I don't understand why it would have to return to the caller to unprotect 
the directory. The iterator would simply drop the reference to the 
previous directory, take a reference on the next one, and keep searching 
for a valid entry.

Why would pt_iter_fini have to walk over the entire range? The iterator 
would keep at most one directory per level referenced. _fini would walk 
the per-level ptd array and unprotect each level, the same way it does 
now.


> 
> So really with what you are asking it whould be:
> 
> hmm_pt_iter_init(&iter, start, end);
> for(next=pt_iter_next(&iter,&ptep); next<end; next=pt_iter_next(&iter,&ptep))
> {
>    // Here ptep is valid until next address. Above you have to call
>    // pt_iter_next() to switch to next directory.
>    addr = max(start, next - (~HMM_PMD_MASK + 1));
>    for (; addr < next; addr += PAGE_SIZE, ptep++) {
>       // access ptep
>    }
> }
> 
> My point is that internally pt_iter_next() will do the exact same test it is
> doing now btw cur and addr. Just that the addr is no longer explicit but iter
> infer it.

But this way, the iteration across directories is more efficient because 
the iterator can simply walk the directory array. Take a directory that 
has one valid entry at the very end. The existing iteration will do this:

hmm_pt_iter_next(dir_addr[0], end)
    Walk up the ptd array
    Compute level start and end and compare them to dir_addr[0]
    Compute dir_addr[1] using addr and pt->mask
    Return dir_addr[1]
hmm_pt_iter_update(dir_addr[1])
    Walk up the ptd array, compute level start and end
    Compute level index of dir_addr[1]
    Read entry for dir_addr[1]
    Return NULL
hmm_pt_iter_next(dir_addr[1], end)
    ...
And so on 511 times until the last entry is read.

This is really more suited to a for loop iteration, which it could be if 
this were fully contained within the _next call.

> 
> > If _next only returned to the caller when it hit a valid hmm_pte (or end), 
> > then only one function would be needed (_next) instead of two 
> > (_update/_walk and _next).
> 
> On the valid entry side, this is because when you are walking the page table
> you have no garanty that the entry will not be clear below you (in case of
> concurrent invalidation). The only garanty you have is that if you are able
> to read a valid entry from the update() callback then this entry is valid
> until you get a new update() callback telling you otherwise.
> 
> Cheers,
> Jérôme
>