Re: [kvm-devel] [patch 1/6] mmu_notifier: Core code
On Thu, 31 Jan 2008, Andrea Arcangeli wrote:
> > H.. exit_mmap is only called when the last reference is removed
> > against the mm right? So no tasks are running anymore. No pages are left.
> > Do we need to serialize at all for mmu_notifier_release?
>
> KVM sure doesn't need any locking there. I thought somebody had to
> possibly take a pin on the "mm_count" and pretend to call
> mmu_notifier_register at will until mmdrop was finally called, in a
> out of order fashion given mmu_notifier_release was implemented like
> if the list could change from under it. Note mmdrop != mmput. mmput
> and in turn mm_users is the serialization point if you prefer to drop
> all locking from _release. Nobody must ever attempt a mmu_notifier_*
> after calling mmput for that mm. That should be enough to be
> safe. I'm fine either ways...
exit_mmap (where we call invalidate_all() and release()) is called when
mm_users == 0:
void mmput(struct mm_struct *mm)
{
might_sleep();
if (atomic_dec_and_test(>mm_users)) {
exit_aio(mm);
exit_mmap(mm);
if (!list_empty(>mmlist)) {
spin_lock(_lock);
list_del(>mmlist);
spin_unlock(_lock);
}
put_swap_token(mm);
mmdrop(mm);
}
}
EXPORT_SYMBOL_GPL(mmput);
So there is only a single thread executing at the time when
invalidate_all() is called from exit_mmap(). Then we drop the
pages, and the page tables. After the page tables we call the ->release
method and then remove the vmas.
So even dropping off the mmu_notifier chain in invalidate_all() could be
done without an issue and without locking.
Trouble is if other callbacks attempt the same. Do we need to support the
removal from the mmu_notifier list in invalidate_range()?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [kvm-devel] [patch 1/6] mmu_notifier: Core code
On Wed, Jan 30, 2008 at 03:55:37PM -0800, Christoph Lameter wrote: > On Thu, 31 Jan 2008, Andrea Arcangeli wrote: > > > > I think Andrea's original concept of the lock in the mmu_notifier_head > > > structure was the best. I agree with him that it should be a spinlock > > > instead of the rw_lock. > > > > BTW, I don't see the scalability concern with huge number of tasks: > > the lock is still in the mm, down_write(mm->mmap_sem); oneinstruction; > > up_write(mm->mmap_sem) is always going to scale worse than > > spin_lock(mm->somethingelse); oneinstruction; > > spin_unlock(mm->somethinglese). > > If we put it elsewhere in the mm then we increase the size of the memory > used in the mm_struct. Yes, and it will increase of the same amount of RAM that you pretend everyone to pay even if MMU_NOTIFIER=n after your patch is applied (vs mine that generated 0 ram utilization increase when MMU_NOTIFIER=n). And the additional ram will provide not just self-contained locking but higher scalability too. I think it's much more important to generate zero ram and CPU overhead for the embedded (this is something I was very careful to enforce in all my patches), than to reduce scalability and not having a self contained locking on full configurations with MMU_NOTIFIER=y. > H.. exit_mmap is only called when the last reference is removed > against the mm right? So no tasks are running anymore. No pages are left. > Do we need to serialize at all for mmu_notifier_release? KVM sure doesn't need any locking there. I thought somebody had to possibly take a pin on the "mm_count" and pretend to call mmu_notifier_register at will until mmdrop was finally called, in a out of order fashion given mmu_notifier_release was implemented like if the list could change from under it. Note mmdrop != mmput. mmput and in turn mm_users is the serialization point if you prefer to drop all locking from _release. Nobody must ever attempt a mmu_notifier_* after calling mmput for that mm. That should be enough to be safe. I'm fine either ways... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [kvm-devel] [patch 1/6] mmu_notifier: Core code
On Wed, Jan 30, 2008 at 03:55:37PM -0800, Christoph Lameter wrote: On Thu, 31 Jan 2008, Andrea Arcangeli wrote: I think Andrea's original concept of the lock in the mmu_notifier_head structure was the best. I agree with him that it should be a spinlock instead of the rw_lock. BTW, I don't see the scalability concern with huge number of tasks: the lock is still in the mm, down_write(mm-mmap_sem); oneinstruction; up_write(mm-mmap_sem) is always going to scale worse than spin_lock(mm-somethingelse); oneinstruction; spin_unlock(mm-somethinglese). If we put it elsewhere in the mm then we increase the size of the memory used in the mm_struct. Yes, and it will increase of the same amount of RAM that you pretend everyone to pay even if MMU_NOTIFIER=n after your patch is applied (vs mine that generated 0 ram utilization increase when MMU_NOTIFIER=n). And the additional ram will provide not just self-contained locking but higher scalability too. I think it's much more important to generate zero ram and CPU overhead for the embedded (this is something I was very careful to enforce in all my patches), than to reduce scalability and not having a self contained locking on full configurations with MMU_NOTIFIER=y. H.. exit_mmap is only called when the last reference is removed against the mm right? So no tasks are running anymore. No pages are left. Do we need to serialize at all for mmu_notifier_release? KVM sure doesn't need any locking there. I thought somebody had to possibly take a pin on the mm_count and pretend to call mmu_notifier_register at will until mmdrop was finally called, in a out of order fashion given mmu_notifier_release was implemented like if the list could change from under it. Note mmdrop != mmput. mmput and in turn mm_users is the serialization point if you prefer to drop all locking from _release. Nobody must ever attempt a mmu_notifier_* after calling mmput for that mm. That should be enough to be safe. I'm fine either ways... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [kvm-devel] [patch 1/6] mmu_notifier: Core code
On Thu, 31 Jan 2008, Andrea Arcangeli wrote:
H.. exit_mmap is only called when the last reference is removed
against the mm right? So no tasks are running anymore. No pages are left.
Do we need to serialize at all for mmu_notifier_release?
KVM sure doesn't need any locking there. I thought somebody had to
possibly take a pin on the mm_count and pretend to call
mmu_notifier_register at will until mmdrop was finally called, in a
out of order fashion given mmu_notifier_release was implemented like
if the list could change from under it. Note mmdrop != mmput. mmput
and in turn mm_users is the serialization point if you prefer to drop
all locking from _release. Nobody must ever attempt a mmu_notifier_*
after calling mmput for that mm. That should be enough to be
safe. I'm fine either ways...
exit_mmap (where we call invalidate_all() and release()) is called when
mm_users == 0:
void mmput(struct mm_struct *mm)
{
might_sleep();
if (atomic_dec_and_test(mm-mm_users)) {
exit_aio(mm);
exit_mmap(mm);
if (!list_empty(mm-mmlist)) {
spin_lock(mmlist_lock);
list_del(mm-mmlist);
spin_unlock(mmlist_lock);
}
put_swap_token(mm);
mmdrop(mm);
}
}
EXPORT_SYMBOL_GPL(mmput);
So there is only a single thread executing at the time when
invalidate_all() is called from exit_mmap(). Then we drop the
pages, and the page tables. After the page tables we call the -release
method and then remove the vmas.
So even dropping off the mmu_notifier chain in invalidate_all() could be
done without an issue and without locking.
Trouble is if other callbacks attempt the same. Do we need to support the
removal from the mmu_notifier list in invalidate_range()?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
