lkml.org 
[lkml]   [2011]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] fix pgd_lock deadlock
    On Tue, Feb 22, 2011 at 02:22:53PM +0000, Jan Beulich wrote:
    > >>> On 22.02.11 at 14:49, Andrea Arcangeli <aarcange@redhat.com> wrote:
    > > On Tue, Feb 22, 2011 at 07:48:54AM +0000, Jan Beulich wrote:
    > >> A possible alternative would be to acquire the page table lock
    > >> in vmalloc_sync_all() only in the Xen case (perhaps by storing
    > >> NULL into page->index in pgd_set_mm() when not running on
    > >> Xen). This is utilizing the fact that there aren't (supposed to
    > >> be - for non-pvops this is definitely the case) any TLB flush IPIs
    > >> under Xen, and hence the race you're trying to fix doesn't
    > >> exist there (while non-Xen doesn't need the extra locking).
    > >
    > > That's sure ok with me. Can we use a global runtime to check if the
    > > guest is running under Xen paravirt, instead of passing that info
    > > through page->something?
    >
    > If everyone's okay with putting a couple of "if (xen_pv_domain())"
    > into mm/fault.c - sure. I would have thought that this wouldn't be
    > liked, hence the suggestion to make this depend on seeing the
    > backlink be non-NULL.

    What about this? The page->private logic gets optimized away at
    compile time with XEN=n.

    The removal of _irqsave from pgd_lock, I'll delay it as it's no bugfix
    anymore.

    ===
    Subject: xen: stop taking the page_table_lock with irq disabled

    From: Andrea Arcangeli <aarcange@redhat.com>

    It's forbidden to take the page_table_lock with the irq disabled or if there's
    contention the IPIs (for tlb flushes) sent with the page_table_lock held will
    never run leading to a deadlock.

    Only Xen needs the page_table_lock and Xen won't need IPI TLB flushes hence the
    deadlock doesn't exist for Xen.

    Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
    ---
    arch/x86/include/asm/pgtable.h | 5 +++--
    arch/x86/mm/fault.c | 10 ++++++----
    arch/x86/mm/init_64.c | 10 ++++++----
    arch/x86/mm/pgtable.c | 9 +++------
    4 files changed, 18 insertions(+), 16 deletions(-)

    --- a/arch/x86/mm/fault.c
    +++ b/arch/x86/mm/fault.c
    @@ -235,14 +235,16 @@ void vmalloc_sync_all(void)

    spin_lock_irqsave(&pgd_lock, flags);
    list_for_each_entry(page, &pgd_list, lru) {
    - spinlock_t *pgt_lock;
    + struct mm_struct *mm;
    pmd_t *ret;

    - pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
    + mm = pgd_page_get_mm(page);

    - spin_lock(pgt_lock);
    + if (mm)
    + spin_lock(&mm->page_table_lock);
    ret = vmalloc_sync_one(page_address(page), address);
    - spin_unlock(pgt_lock);
    + if (mm)
    + spin_unlock(&mm->page_table_lock);

    if (!ret)
    break;
    --- a/arch/x86/mm/init_64.c
    +++ b/arch/x86/mm/init_64.c
    @@ -114,11 +114,12 @@ void sync_global_pgds(unsigned long star
    spin_lock_irqsave(&pgd_lock, flags);
    list_for_each_entry(page, &pgd_list, lru) {
    pgd_t *pgd;
    - spinlock_t *pgt_lock;
    + struct mm_struct *mm;

    pgd = (pgd_t *)page_address(page) + pgd_index(address);
    - pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
    - spin_lock(pgt_lock);
    + mm = pgd_page_get_mm(page);
    + if (mm)
    + spin_lock(&mm->page_table_lock);

    if (pgd_none(*pgd))
    set_pgd(pgd, *pgd_ref);
    @@ -126,7 +127,8 @@ void sync_global_pgds(unsigned long star
    BUG_ON(pgd_page_vaddr(*pgd)
    != pgd_page_vaddr(*pgd_ref));

    - spin_unlock(pgt_lock);
    + if (mm)
    + spin_unlock(&mm->page_table_lock);
    }
    spin_unlock_irqrestore(&pgd_lock, flags);
    }
    --- a/arch/x86/mm/pgtable.c
    +++ b/arch/x86/mm/pgtable.c
    @@ -4,6 +4,7 @@
    #include <asm/pgtable.h>
    #include <asm/tlb.h>
    #include <asm/fixmap.h>
    +#include <xen/xen.h>

    #define PGALLOC_GFP GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO

    @@ -91,12 +92,8 @@ static inline void pgd_list_del(pgd_t *p
    static void pgd_set_mm(pgd_t *pgd, struct mm_struct *mm)
    {
    BUILD_BUG_ON(sizeof(virt_to_page(pgd)->index) < sizeof(mm));
    - virt_to_page(pgd)->index = (pgoff_t)mm;
    -}
    -
    -struct mm_struct *pgd_page_get_mm(struct page *page)
    -{
    - return (struct mm_struct *)page->index;
    + if (xen_pv_domain())
    + virt_to_page(pgd)->index = (pgoff_t)mm;
    }

    static void pgd_ctor(struct mm_struct *mm, pgd_t *pgd)
    --- a/arch/x86/include/asm/pgtable.h
    +++ b/arch/x86/include/asm/pgtable.h
    @@ -28,8 +28,6 @@ extern unsigned long empty_zero_page[PAG
    extern spinlock_t pgd_lock;
    extern struct list_head pgd_list;

    -extern struct mm_struct *pgd_page_get_mm(struct page *page);
    -
    #ifdef CONFIG_PARAVIRT
    #include <asm/paravirt.h>
    #else /* !CONFIG_PARAVIRT */
    @@ -83,6 +81,9 @@ extern struct mm_struct *pgd_page_get_mm

    #endif /* CONFIG_PARAVIRT */

    +#define pgd_page_get_mm(__page) \
    + ((struct mm_struct *)(xen_pv_domain() ? (__page)->index : 0))
    +
    /*
    * The following only work if pte_present() is true.
    * Undefined behaviour if not..

    \
     
     \ /
      Last update: 2011-02-24 05:25    [W:0.037 / U:0.628 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site