Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-05-07 Thread Andrea Arcangeli
On Tue, Apr 29, 2008 at 06:03:40PM +0200, Andrea Arcangeli wrote: Christoph if you've interest in evolving anon-vma-sem and i_mmap_sem yourself in this direction, you're very welcome to go ahead while I In case you didn't notice this already, for a further explanation of why semaphores runs

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-29 Thread Hugh Dickins
On Tue, 29 Apr 2008, Andrea Arcangeli wrote: My point of view is that there was no rcu when I wrote that code, yet there was no reference count and yet all locking looks still exactly the same as I wrote it. There's even still the page_table_lock to serialize threads taking the mmap_sem in

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-29 Thread Andrea Arcangeli
Hi Hugh!! On Tue, Apr 29, 2008 at 11:49:11AM +0100, Hugh Dickins wrote: [I'm scarcely following the mmu notifiers to-and-fro, which seems to be in good hands, amongst faster thinkers than me: who actually need and can test this stuff. Don't let me slow you down; but I can quickly clarify on

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-29 Thread Andrea Arcangeli
On Mon, Apr 28, 2008 at 06:28:06PM -0700, Christoph Lameter wrote: On Tue, 29 Apr 2008, Andrea Arcangeli wrote: Frankly I've absolutely no idea why rcu is needed in all rmap code when walking the page-mapping. Definitely the PG_locked is taken so there's no way page-mapping could possibly

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-29 Thread Robin Holt
I however doubt this will bring us back to the same performance of the current spinlock version, as the real overhead should come out of overscheduling in down_write ai anon_vma_link. Here an initially spinning lock would help but that's gray area, it greatly depends on timings, and on very

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-29 Thread Andrea Arcangeli
On Tue, Apr 29, 2008 at 10:50:30AM -0500, Robin Holt wrote: You have said this continually about a CONFIG option. I am unsure how that could be achieved. Could you provide a patch? I'm busy with the reserved ram patch against 2.6.25 and latest kvm.git that is moving from pages to pfn for pci

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-28 Thread Christoph Lameter
On Sun, 27 Apr 2008, Andrea Arcangeli wrote: Talking about post 2.6.26: the refcount with rcu in the anon-vma conversion seems unnecessary and may explain part of the AIM slowdown too. The rest looks ok and probably we should switch the code to a compile-time decision between rwlock and rwsem

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-28 Thread Andrea Arcangeli
On Mon, Apr 28, 2008 at 01:34:11PM -0700, Christoph Lameter wrote: On Sun, 27 Apr 2008, Andrea Arcangeli wrote: Talking about post 2.6.26: the refcount with rcu in the anon-vma conversion seems unnecessary and may explain part of the AIM slowdown too. The rest looks ok and probably we

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-27 Thread Andrea Arcangeli
On Sat, Apr 26, 2008 at 08:17:34AM -0500, Robin Holt wrote: the first four sets. The fifth is the oversubscription test which trips my xpmem bug. This is as good as the v12 runs from before. Now that mmu-notifier-core #v14 seems finished and hopefully will appear in 2.6.26 ;), I started

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-26 Thread Robin Holt
On Thu, Apr 24, 2008 at 07:41:45PM +0200, Andrea Arcangeli wrote: A full new update will some become visible here: http://www.kernel.org/pub/linux/kernel/people/andrea/patches/v2.6/2.6.25/mmu-notifier-v14-pre3/ I grabbed these and built them. Only change needed was another include.

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-26 Thread Andrea Arcangeli
On Sat, Apr 26, 2008 at 08:17:34AM -0500, Robin Holt wrote: Since this include and the one for mm_types.h both are build breakages for ia64, I think you need to apply your ia64_cpumask and the following (possibly as a single patch) first or in your patch 1. Without that, ia64 doing a

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-24 Thread Andrea Arcangeli
On Thu, Apr 24, 2008 at 12:19:28AM +0200, Andrea Arcangeli wrote: /dev/kvm closure. Given this can be a considered a bugfix to mmu_notifier_unregister I'll apply it to 1/N and I'll release a new I'm not sure anymore this can be considered a bugfix given how large change this resulted in the

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-24 Thread Robin Holt
I am not certain of this, but it seems like this patch leaves things in a somewhat asymetric state. At the very least, I think that asymetry should be documented in the comments of either mmu_notifier.h or .c. Before I do the first mmu_notifier_register, all places that test for

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-24 Thread Andrea Arcangeli
On Thu, Apr 24, 2008 at 04:51:12AM -0500, Robin Holt wrote: It seems to me the work done by mmu_notifier_mm_destroy should really be done inside the mm_lock()/mm_unlock area of mmu_unregister and There's no mm_lock/unlock for mmu_unregister anymore. That's the whole point of using srcu so it

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-24 Thread Andrea Arcangeli
On Thu, Apr 24, 2008 at 05:39:43PM +0200, Andrea Arcangeli wrote: There's at least one small issue I noticed so far, that while _release don't need to care about _register, but _unregister definitely need to care about _register. I've to take the mmap_sem in addition or in In the end the best

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Tue, Apr 22, 2008 at 06:07:27PM -0500, Robin Holt wrote: The only other change I did has been to move mmu_notifier_unregister at the end of the patchset after getting more questions about its reliability and I documented a bit the rmmod requirements for -release. we'll think later if it

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Robin Holt
On Wed, Apr 23, 2008 at 03:36:19PM +0200, Andrea Arcangeli wrote: On Tue, Apr 22, 2008 at 06:07:27PM -0500, Robin Holt wrote: The only other change I did has been to move mmu_notifier_unregister at the end of the patchset after getting more questions about its reliability and I

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 09:47:47AM -0500, Robin Holt wrote: It also makes the API consistent. What you are proposing is equivalent to having a file you can open but never close. That's not entirely true, you can close the file just fine it by killing the tasks leading to an mmput. From an user

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Tue, Apr 22, 2008 at 04:20:35PM -0700, Christoph Lameter wrote: I guess I have to prepare another patchset then? If you want to embarrass yourself three time in a row go ahead ;). I thought two failed takeovers was enough.

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Tue, Apr 22, 2008 at 07:28:49PM -0500, Jack Steiner wrote: The GRU driver unregisters the notifier when all GRU mappings are unmapped. I could make it work either way - either with or without an unregister function. However, unregister is the most logical action to take when all mappings

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Jack Steiner
You may have spotted this already. If so, just ignore this. It looks like there is a bug in copy_page_range() around line 667. It's possible to do a mmu_notifier_invalidate_range_start(), then return -ENOMEM w/o doing a corresponding mmu_notifier_invalidate_range_end(). --- jack

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 06:26:29PM +0200, Andrea Arcangeli wrote: On Tue, Apr 22, 2008 at 04:20:35PM -0700, Christoph Lameter wrote: I guess I have to prepare another patchset then? Apologies for my previous not too polite comment in answer to the above, but I thought this double patchset was

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 12:09:09PM -0500, Jack Steiner wrote: You may have spotted this already. If so, just ignore this. It looks like there is a bug in copy_page_range() around line 667. It's possible to do a mmu_notifier_invalidate_range_start(), then return -ENOMEM w/o doing a

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Christoph Lameter
On Wed, 23 Apr 2008, Andrea Arcangeli wrote: Implement unregister but it's not reliable, only -release is reliable. Why is there still the hlist stuff being used for the mmu notifier list? And why is this still unsafe? There are cases in which you do not take the reverse map locks or mmap_sem

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Christoph Lameter
On Wed, 23 Apr 2008, Andrea Arcangeli wrote: On Tue, Apr 22, 2008 at 04:20:35PM -0700, Christoph Lameter wrote: I guess I have to prepare another patchset then? If you want to embarrass yourself three time in a row go ahead ;). I thought two failed takeovers was enough. Takeover? I'd be

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Christoph Lameter
On Wed, 23 Apr 2008, Andrea Arcangeli wrote: The only way to avoid failing because of vmalloc space shortage or oom, would be to provide a O(N*N) fallback. But one that can't be interrupted by sigkill! sigkill interruption was ok in #v12 because we didn't rely on mmu_notifier_unregister to

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 11:09:35AM -0700, Christoph Lameter wrote: Why is there still the hlist stuff being used for the mmu notifier list? And why is this still unsafe? What's the problem with hlist, it saves 8 bytes for each mm_struct, you should be using it too instead of list. There are

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Christoph Lameter
On Wed, 23 Apr 2008, Andrea Arcangeli wrote: will go in -mm in time for 2.6.26. Let's put it this way, if I fail to merge mmu-notifier-core into 2.6.26 I'll voluntarily give up my entire patchset and leave maintainership to you so you move 1/N to N/N and remove mm_lock-sem patch (everything

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 11:19:26AM -0700, Christoph Lameter wrote: If unregister fails then the driver should not detach from the address space immediately but wait until --release is called. That may be a possible solution. It will be rare that the unregister fails. This is the current idea,

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Christoph Lameter
On Wed, 23 Apr 2008, Andrea Arcangeli wrote: On Wed, Apr 23, 2008 at 11:09:35AM -0700, Christoph Lameter wrote: Why is there still the hlist stuff being used for the mmu notifier list? And why is this still unsafe? What's the problem with hlist, it saves 8 bytes for each mm_struct, you

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 11:21:49AM -0700, Christoph Lameter wrote: No I really want you to do this. I have no interest in a takeover in the Ok if you want me to do this, I definitely prefer the core to go in now. It's so much easier to concentrate on two problems at different times then to

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 11:27:21AM -0700, Christoph Lameter wrote: There is a potential issue in move_ptes where you call invalidate_range_end after dropping i_mmap_sem whereas my patches did the opposite. Mmap_sem saves you there? Yes, there's really no risk of races in this area after

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Christoph Lameter
On Wed, 23 Apr 2008, Andrea Arcangeli wrote: Yes, there's really no risk of races in this area after introducing mm_lock, any place that mangles over ptes and doesn't hold any of the three locks is buggy anyway. I appreciate the audit work (I also did it and couldn't find bugs but the more

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-23 Thread Andrea Arcangeli
On Wed, Apr 23, 2008 at 06:37:13PM +0200, Andrea Arcangeli wrote: I'm afraid if you don't want to worst-case unregister with -release you need to have a better idea than my mm_lock and personally I can't see any other way than mm_lock to ensure not to miss range_begin... But wait,

[kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Andrea Arcangeli
# HG changeset patch # User Andrea Arcangeli [EMAIL PROTECTED] # Date 1208870142 -7200 # Node ID ea87c15371b1bd49380c40c3f15f1c7ca4438af5 # Parent fb3bc9942fb78629d096bd07564f435d51d86e5f Core of mmu notifiers. Signed-off-by: Andrea Arcangeli [EMAIL PROTECTED] Signed-off-by: Nick Piggin [EMAIL

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Andrea Arcangeli
On Tue, Apr 22, 2008 at 04:56:10PM +0200, Eric Dumazet wrote: Andrea Arcangeli a écrit : + +static int mm_lock_cmp(const void *a, const void *b) +{ +cond_resched(); +if ((unsigned long)*(spinlock_t **)a +(unsigned long)*(spinlock_t **)b) +return -1; +else

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Avi Kivity
Andrea Arcangeli wrote: On Tue, Apr 22, 2008 at 04:56:10PM +0200, Eric Dumazet wrote: Andrea Arcangeli a écrit : + +static int mm_lock_cmp(const void *a, const void *b) +{ + cond_resched(); + if ((unsigned long)*(spinlock_t **)a + (unsigned long)*(spinlock_t **)b) +

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Eric Dumazet
Andrea Arcangeli a écrit : On Tue, Apr 22, 2008 at 04:56:10PM +0200, Eric Dumazet wrote: Andrea Arcangeli a écrit : + +static int mm_lock_cmp(const void *a, const void *b) +{ + cond_resched(); + if ((unsigned long)*(spinlock_t **)a + (unsigned long)*(spinlock_t **)b)

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Andrea Arcangeli
On Tue, Apr 22, 2008 at 05:37:38PM +0200, Eric Dumazet wrote: I am saying your intent was probably to test else if ((unsigned long)*(spinlock_t **)a == (unsigned long)*(spinlock_t **)b) return 0; Indeed... Hum, it's not a micro-optimization, but a bug fix. :) The

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Eric Dumazet
Andrea Arcangeli a écrit : + +static int mm_lock_cmp(const void *a, const void *b) +{ + cond_resched(); + if ((unsigned long)*(spinlock_t **)a + (unsigned long)*(spinlock_t **)b) + return -1; + else if (a == b) + return 0; + else +

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Christoph Lameter
Thanks for adding most of my enhancements. But 1. There is no real need for invalidate_page(). Can be done with invalidate_start/end. Needlessly complicates the API. One of the objections by Andrew was that there mere multiple callbacks that perform similar functions. 2.

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Robin Holt
On Tue, Apr 22, 2008 at 01:19:29PM -0700, Christoph Lameter wrote: Thanks for adding most of my enhancements. But 1. There is no real need for invalidate_page(). Can be done with invalidate_start/end. Needlessly complicates the API. One of the objections by Andrew was that there

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Andrea Arcangeli
On Tue, Apr 22, 2008 at 01:19:29PM -0700, Christoph Lameter wrote: 3. As noted by Eric and also contained in private post from yesterday by me: The cmp function needs to retrieve the value before doing comparisons which is not done for the == of a and b. I retrieved the value, which is

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Robin Holt
The only other change I did has been to move mmu_notifier_unregister at the end of the patchset after getting more questions about its reliability and I documented a bit the rmmod requirements for -release. we'll think later if it makes sense to add it, nobody's using it anyway. XPMEM is

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Christoph Lameter
On Wed, 23 Apr 2008, Andrea Arcangeli wrote: I'll send an update in any case to Andrew way before Saturday so hopefully we'll finally get mmu-notifiers-core merged before next week. Also I'm not updating my mmu-notifier-core patch anymore except for strict bugfixes so don't worry about any

Re: [kvm-devel] [PATCH 01 of 12] Core of mmu notifiers

2008-04-22 Thread Jack Steiner
On Tue, Apr 22, 2008 at 06:07:27PM -0500, Robin Holt wrote: The only other change I did has been to move mmu_notifier_unregister at the end of the patchset after getting more questions about its reliability and I documented a bit the rmmod requirements for -release. we'll think later if it