* [patch 3/5]thp: add tlb_remove_pmd_tlb_entry
@ 2011-10-25 2:59 Shaohua Li
2011-11-10 15:36 ` Andrea Arcangeli
0 siblings, 1 reply; 5+ messages in thread
From: Shaohua Li @ 2011-10-25 2:59 UTC (permalink / raw)
To: Andrew Morton; +Cc: aarcange, Hugh Dickins, linux-mm, lkml
We have tlb_remove_tlb_entry to indicate a pte tlb flush entry should be
flushed, but not a corresponding API for pmd entry. This isn't a problem so far
because THP is only for x86 currently and tlb_flush() under x86 will flush
entire TLB. But thsi is confusion and could be missed if thp is ported to
other arch.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
arch/x86/include/asm/tlb.h | 1 +
include/asm-generic/tlb.h | 10 ++++++++++
include/linux/huge_mm.h | 2 +-
mm/huge_memory.c | 3 ++-
mm/memory.c | 2 +-
5 files changed, 15 insertions(+), 3 deletions(-)
Index: linux/arch/x86/include/asm/tlb.h
===================================================================
--- linux.orig/arch/x86/include/asm/tlb.h 2011-10-25 09:00:39.000000000 +0800
+++ linux/arch/x86/include/asm/tlb.h 2011-10-25 09:02:52.000000000 +0800
@@ -4,6 +4,7 @@
#define tlb_start_vma(tlb, vma) do { } while (0)
#define tlb_end_vma(tlb, vma) do { } while (0)
#define __tlb_remove_tlb_entry(tlb, ptep, address) do { } while (0)
+#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do { } while (0)
#define tlb_flush(tlb) flush_tlb_mm((tlb)->mm)
#include <asm-generic/tlb.h>
Index: linux/include/asm-generic/tlb.h
===================================================================
--- linux.orig/include/asm-generic/tlb.h 2011-10-25 09:00:23.000000000 +0800
+++ linux/include/asm-generic/tlb.h 2011-10-25 09:18:01.000000000 +0800
@@ -139,6 +139,16 @@ static inline void tlb_remove_page(struc
__tlb_remove_tlb_entry(tlb, ptep, address); \
} while (0)
+#ifndef __tlb_remove_pmd_tlb_entry
+#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do {} while(0)
+#endif
+
+#define tlb_remove_pmd_tlb_entry(tlb, pmdp, address) \
+ do { \
+ tlb->need_flush = 1; \
+ __tlb_remove_pmd_tlb_entry(tlb, pmdp, address); \
+ } while (0)
+
#define pte_free_tlb(tlb, ptep, address) \
do { \
tlb->need_flush = 1; \
Index: linux/include/linux/huge_mm.h
===================================================================
--- linux.orig/include/linux/huge_mm.h 2011-10-25 09:07:12.000000000 +0800
+++ linux/include/linux/huge_mm.h 2011-10-25 09:07:44.000000000 +0800
@@ -18,7 +18,7 @@ extern struct page *follow_trans_huge_pm
unsigned int flags);
extern int zap_huge_pmd(struct mmu_gather *tlb,
struct vm_area_struct *vma,
- pmd_t *pmd);
+ pmd_t *pmd, unsigned long addr);
extern int mincore_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
unsigned long addr, unsigned long end,
unsigned char *vec);
Index: linux/mm/huge_memory.c
===================================================================
--- linux.orig/mm/huge_memory.c 2011-10-25 09:00:07.000000000 +0800
+++ linux/mm/huge_memory.c 2011-10-25 09:06:55.000000000 +0800
@@ -1005,7 +1005,7 @@ out:
}
int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
- pmd_t *pmd)
+ pmd_t *pmd, unsigned long addr)
{
int ret = 0;
@@ -1021,6 +1021,7 @@ int zap_huge_pmd(struct mmu_gather *tlb,
pgtable = get_pmd_huge_pte(tlb->mm);
page = pmd_page(*pmd);
pmd_clear(pmd);
+ tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
page_remove_rmap(page);
VM_BUG_ON(page_mapcount(page) < 0);
add_mm_counter(tlb->mm, MM_ANONPAGES, -HPAGE_PMD_NR);
Index: linux/mm/memory.c
===================================================================
--- linux.orig/mm/memory.c 2011-10-25 09:07:49.000000000 +0800
+++ linux/mm/memory.c 2011-10-25 09:08:29.000000000 +0800
@@ -1231,7 +1231,7 @@ static inline unsigned long zap_pmd_rang
if (next-addr != HPAGE_PMD_SIZE) {
VM_BUG_ON(!rwsem_is_locked(&tlb->mm->mmap_sem));
split_huge_page_pmd(vma->vm_mm, pmd);
- } else if (zap_huge_pmd(tlb, vma, pmd))
+ } else if (zap_huge_pmd(tlb, vma, pmd, addr))
continue;
/* fall through */
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 3/5]thp: add tlb_remove_pmd_tlb_entry
2011-10-25 2:59 [patch 3/5]thp: add tlb_remove_pmd_tlb_entry Shaohua Li
@ 2011-11-10 15:36 ` Andrea Arcangeli
2011-11-11 6:36 ` Shaohua Li
0 siblings, 1 reply; 5+ messages in thread
From: Andrea Arcangeli @ 2011-11-10 15:36 UTC (permalink / raw)
To: Shaohua Li; +Cc: Andrew Morton, Hugh Dickins, linux-mm, lkml
On Tue, Oct 25, 2011 at 10:59:31AM +0800, Shaohua Li wrote:
> Index: linux/arch/x86/include/asm/tlb.h
> ===================================================================
> --- linux.orig/arch/x86/include/asm/tlb.h 2011-10-25 09:00:39.000000000 +0800
> +++ linux/arch/x86/include/asm/tlb.h 2011-10-25 09:02:52.000000000 +0800
> @@ -4,6 +4,7 @@
> #define tlb_start_vma(tlb, vma) do { } while (0)
> #define tlb_end_vma(tlb, vma) do { } while (0)
> #define __tlb_remove_tlb_entry(tlb, ptep, address) do { } while (0)
> +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do { } while (0)
> #define tlb_flush(tlb) flush_tlb_mm((tlb)->mm)
This is superfluous, it's already define below as noop.
>
> #include <asm-generic/tlb.h>
> Index: linux/include/asm-generic/tlb.h
> ===================================================================
> --- linux.orig/include/asm-generic/tlb.h 2011-10-25 09:00:23.000000000 +0800
> +++ linux/include/asm-generic/tlb.h 2011-10-25 09:18:01.000000000 +0800
> @@ -139,6 +139,16 @@ static inline void tlb_remove_page(struc
> __tlb_remove_tlb_entry(tlb, ptep, address); \
> } while (0)
>
> +#ifndef __tlb_remove_pmd_tlb_entry
> +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do {} while(0)
> +#endif
> +
> +#define tlb_remove_pmd_tlb_entry(tlb, pmdp, address) \
> + do { \
> + tlb->need_flush = 1; \
> + __tlb_remove_pmd_tlb_entry(tlb, pmdp, address); \
> + } while (0)
this looks weird, why do we set need_flush = 1 again, considering that
we're doing tlb_remove_page() just a few lines later (which also sets
tlb->need_flush = 1).
Ok that other archs may need the __tlb_remove_pmd_tlb_entry to be
called (and I've no idea why), but the need_flush = 1 seems
unnecessary.
Why other archs need the __tlb_remove_pmd_tlb_entry to be called?
One way to go would be to change the tlb->need_flush = 1 in
__tlb_remove_page to a VM_BUG_ON(!tlb->need_flush) and then we keep it
above and we add the __tlb_remove_pmd_tlb_entry call.
Or is there any place where __tlb_remove_page is called without a
tlb_remove_*tlb_entry being called before it?
In any case the VM_BUG_ON will verify this.
> Index: linux/include/linux/huge_mm.h
> ===================================================================
> --- linux.orig/include/linux/huge_mm.h 2011-10-25 09:07:12.000000000 +0800
> +++ linux/include/linux/huge_mm.h 2011-10-25 09:07:44.000000000 +0800
> @@ -18,7 +18,7 @@ extern struct page *follow_trans_huge_pm
> unsigned int flags);
> extern int zap_huge_pmd(struct mmu_gather *tlb,
> struct vm_area_struct *vma,
> - pmd_t *pmd);
> + pmd_t *pmd, unsigned long addr);
> extern int mincore_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> unsigned long addr, unsigned long end,
> unsigned char *vec);
> Index: linux/mm/huge_memory.c
> ===================================================================
> --- linux.orig/mm/huge_memory.c 2011-10-25 09:00:07.000000000 +0800
> +++ linux/mm/huge_memory.c 2011-10-25 09:06:55.000000000 +0800
> @@ -1005,7 +1005,7 @@ out:
> }
>
> int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> - pmd_t *pmd)
> + pmd_t *pmd, unsigned long addr)
> {
> int ret = 0;
>
> @@ -1021,6 +1021,7 @@ int zap_huge_pmd(struct mmu_gather *tlb,
> pgtable = get_pmd_huge_pte(tlb->mm);
> page = pmd_page(*pmd);
> pmd_clear(pmd);
> + tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
> page_remove_rmap(page);
> VM_BUG_ON(page_mapcount(page) < 0);
> add_mm_counter(tlb->mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> Index: linux/mm/memory.c
> ===================================================================
> --- linux.orig/mm/memory.c 2011-10-25 09:07:49.000000000 +0800
> +++ linux/mm/memory.c 2011-10-25 09:08:29.000000000 +0800
> @@ -1231,7 +1231,7 @@ static inline unsigned long zap_pmd_rang
> if (next-addr != HPAGE_PMD_SIZE) {
> VM_BUG_ON(!rwsem_is_locked(&tlb->mm->mmap_sem));
> split_huge_page_pmd(vma->vm_mm, pmd);
> - } else if (zap_huge_pmd(tlb, vma, pmd))
> + } else if (zap_huge_pmd(tlb, vma, pmd, addr))
> continue;
> /* fall through */
> }
Rest looks ok.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 3/5]thp: add tlb_remove_pmd_tlb_entry
2011-11-10 15:36 ` Andrea Arcangeli
@ 2011-11-11 6:36 ` Shaohua Li
2011-11-11 6:53 ` Andrea Arcangeli
0 siblings, 1 reply; 5+ messages in thread
From: Shaohua Li @ 2011-11-11 6:36 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: Andrew Morton, Hugh Dickins, linux-mm, lkml
On Thu, 2011-11-10 at 23:36 +0800, Andrea Arcangeli wrote:
> On Tue, Oct 25, 2011 at 10:59:31AM +0800, Shaohua Li wrote:
> > Index: linux/arch/x86/include/asm/tlb.h
> > ===================================================================
> > --- linux.orig/arch/x86/include/asm/tlb.h 2011-10-25 09:00:39.000000000 +0800
> > +++ linux/arch/x86/include/asm/tlb.h 2011-10-25 09:02:52.000000000 +0800
> > @@ -4,6 +4,7 @@
> > #define tlb_start_vma(tlb, vma) do { } while (0)
> > #define tlb_end_vma(tlb, vma) do { } while (0)
> > #define __tlb_remove_tlb_entry(tlb, ptep, address) do { } while (0)
> > +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do { } while (0)
> > #define tlb_flush(tlb) flush_tlb_mm((tlb)->mm)
>
> This is superfluous, it's already define below as noop.
>
> >
> > #include <asm-generic/tlb.h>
> > Index: linux/include/asm-generic/tlb.h
> > ===================================================================
> > --- linux.orig/include/asm-generic/tlb.h 2011-10-25 09:00:23.000000000 +0800
> > +++ linux/include/asm-generic/tlb.h 2011-10-25 09:18:01.000000000 +0800
> > @@ -139,6 +139,16 @@ static inline void tlb_remove_page(struc
> > __tlb_remove_tlb_entry(tlb, ptep, address); \
> > } while (0)
> >
> > +#ifndef __tlb_remove_pmd_tlb_entry
> > +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do {} while(0)
> > +#endif
> > +
> > +#define tlb_remove_pmd_tlb_entry(tlb, pmdp, address) \
> > + do { \
> > + tlb->need_flush = 1; \
> > + __tlb_remove_pmd_tlb_entry(tlb, pmdp, address); \
> > + } while (0)
>
> this looks weird, why do we set need_flush = 1 again, considering that
> we're doing tlb_remove_page() just a few lines later (which also sets
> tlb->need_flush = 1).
>
> Ok that other archs may need the __tlb_remove_pmd_tlb_entry to be
> called (and I've no idea why), but the need_flush = 1 seems
> unnecessary.
>
> Why other archs need the __tlb_remove_pmd_tlb_entry to be called?
>
> One way to go would be to change the tlb->need_flush = 1 in
> __tlb_remove_page to a VM_BUG_ON(!tlb->need_flush) and then we keep it
> above and we add the __tlb_remove_pmd_tlb_entry call.
>
> Or is there any place where __tlb_remove_page is called without a
> tlb_remove_*tlb_entry being called before it?
>
> In any case the VM_BUG_ON will verify this.
ok, I made the whole tlb_remove_pmd_tlb_entry() noop now. we don't need
add anything on it for x86 currently. We can change it later if
necessary.
We have tlb_remove_tlb_entry to indicate a pte tlb flush entry should be
flushed, but not a corresponding API for pmd entry. This isn't a problem so far
because THP is only for x86 currently and tlb_flush() under x86 will flush
entire TLB. But this is confusion and could be missed if thp is ported to
other arch.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
include/asm-generic/tlb.h | 6 ++++++
include/linux/huge_mm.h | 2 +-
mm/huge_memory.c | 3 ++-
mm/memory.c | 2 +-
4 files changed, 10 insertions(+), 3 deletions(-)
Index: linux/include/asm-generic/tlb.h
===================================================================
--- linux.orig/include/asm-generic/tlb.h 2011-11-11 14:26:33.000000000 +0800
+++ linux/include/asm-generic/tlb.h 2011-11-11 14:26:35.000000000 +0800
@@ -139,6 +139,12 @@ static inline void tlb_remove_page(struc
__tlb_remove_tlb_entry(tlb, ptep, address); \
} while (0)
+/**
+ * tlb_remove_pmd_tlb_entry - remember a pmd mapping for later tlb invalidation
+ * This is a nop so far, because only x86 needs it.
+ */
+#define tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do {} while (0)
+
#define pte_free_tlb(tlb, ptep, address) \
do { \
tlb->need_flush = 1; \
Index: linux/include/linux/huge_mm.h
===================================================================
--- linux.orig/include/linux/huge_mm.h 2011-11-11 14:26:33.000000000 +0800
+++ linux/include/linux/huge_mm.h 2011-11-11 14:26:35.000000000 +0800
@@ -18,7 +18,7 @@ extern struct page *follow_trans_huge_pm
unsigned int flags);
extern int zap_huge_pmd(struct mmu_gather *tlb,
struct vm_area_struct *vma,
- pmd_t *pmd);
+ pmd_t *pmd, unsigned long addr);
extern int mincore_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
unsigned long addr, unsigned long end,
unsigned char *vec);
Index: linux/mm/huge_memory.c
===================================================================
--- linux.orig/mm/huge_memory.c 2011-11-11 14:26:33.000000000 +0800
+++ linux/mm/huge_memory.c 2011-11-11 14:26:35.000000000 +0800
@@ -1026,7 +1026,7 @@ out:
}
int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
- pmd_t *pmd)
+ pmd_t *pmd, unsigned long addr)
{
int ret = 0;
@@ -1042,6 +1042,7 @@ int zap_huge_pmd(struct mmu_gather *tlb,
pgtable = get_pmd_huge_pte(tlb->mm);
page = pmd_page(*pmd);
pmd_clear(pmd);
+ tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
page_remove_rmap(page);
VM_BUG_ON(page_mapcount(page) < 0);
add_mm_counter(tlb->mm, MM_ANONPAGES, -HPAGE_PMD_NR);
Index: linux/mm/memory.c
===================================================================
--- linux.orig/mm/memory.c 2011-11-11 14:26:33.000000000 +0800
+++ linux/mm/memory.c 2011-11-11 14:26:35.000000000 +0800
@@ -1231,7 +1231,7 @@ static inline unsigned long zap_pmd_rang
if (next-addr != HPAGE_PMD_SIZE) {
VM_BUG_ON(!rwsem_is_locked(&tlb->mm->mmap_sem));
split_huge_page_pmd(vma->vm_mm, pmd);
- } else if (zap_huge_pmd(tlb, vma, pmd))
+ } else if (zap_huge_pmd(tlb, vma, pmd, addr))
continue;
/* fall through */
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 3/5]thp: add tlb_remove_pmd_tlb_entry
2011-11-11 6:36 ` Shaohua Li
@ 2011-11-11 6:53 ` Andrea Arcangeli
2011-11-11 8:42 ` Shaohua Li
0 siblings, 1 reply; 5+ messages in thread
From: Andrea Arcangeli @ 2011-11-11 6:53 UTC (permalink / raw)
To: Shaohua Li; +Cc: Andrew Morton, Hugh Dickins, linux-mm, lkml
On Fri, Nov 11, 2011 at 02:36:29PM +0800, Shaohua Li wrote:
> On Thu, 2011-11-10 at 23:36 +0800, Andrea Arcangeli wrote:
> > On Tue, Oct 25, 2011 at 10:59:31AM +0800, Shaohua Li wrote:
> > > Index: linux/arch/x86/include/asm/tlb.h
> > > ===================================================================
> > > --- linux.orig/arch/x86/include/asm/tlb.h 2011-10-25 09:00:39.000000000 +0800
> > > +++ linux/arch/x86/include/asm/tlb.h 2011-10-25 09:02:52.000000000 +0800
> > > @@ -4,6 +4,7 @@
> > > #define tlb_start_vma(tlb, vma) do { } while (0)
> > > #define tlb_end_vma(tlb, vma) do { } while (0)
> > > #define __tlb_remove_tlb_entry(tlb, ptep, address) do { } while (0)
> > > +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do { } while (0)
> > > #define tlb_flush(tlb) flush_tlb_mm((tlb)->mm)
> >
> > This is superfluous, it's already define below as noop.
> >
> > >
> > > #include <asm-generic/tlb.h>
> > > Index: linux/include/asm-generic/tlb.h
> > > ===================================================================
> > > --- linux.orig/include/asm-generic/tlb.h 2011-10-25 09:00:23.000000000 +0800
> > > +++ linux/include/asm-generic/tlb.h 2011-10-25 09:18:01.000000000 +0800
> > > @@ -139,6 +139,16 @@ static inline void tlb_remove_page(struc
> > > __tlb_remove_tlb_entry(tlb, ptep, address); \
> > > } while (0)
> > >
> > > +#ifndef __tlb_remove_pmd_tlb_entry
> > > +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do {} while(0)
> > > +#endif
> > > +
> > > +#define tlb_remove_pmd_tlb_entry(tlb, pmdp, address) \
> > > + do { \
> > > + tlb->need_flush = 1; \
> > > + __tlb_remove_pmd_tlb_entry(tlb, pmdp, address); \
> > > + } while (0)
> >
> > this looks weird, why do we set need_flush = 1 again, considering that
> > we're doing tlb_remove_page() just a few lines later (which also sets
> > tlb->need_flush = 1).
> >
> > Ok that other archs may need the __tlb_remove_pmd_tlb_entry to be
> > called (and I've no idea why), but the need_flush = 1 seems
> > unnecessary.
> >
> > Why other archs need the __tlb_remove_pmd_tlb_entry to be called?
> >
> > One way to go would be to change the tlb->need_flush = 1 in
> > __tlb_remove_page to a VM_BUG_ON(!tlb->need_flush) and then we keep it
> > above and we add the __tlb_remove_pmd_tlb_entry call.
> >
> > Or is there any place where __tlb_remove_page is called without a
> > tlb_remove_*tlb_entry being called before it?
> >
> > In any case the VM_BUG_ON will verify this.
> ok, I made the whole tlb_remove_pmd_tlb_entry() noop now. we don't need
> add anything on it for x86 currently. We can change it later if
> necessary.
I thought it'd be cleaner to have only the __tlb_remove_*tlb_entry
variants set need_flush=1 and have __tlb_remove_page just check that
is set under a VM_BUG_ON. That would also avoid a second unnecessary
need_flush = 1 for the pte case which is repeated now (it's not the
repeated in the pmd case in your patch because it's a noop, but the
pte case it's not a noop). Maybe it's not possible but if it's
possible it looks better.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 3/5]thp: add tlb_remove_pmd_tlb_entry
2011-11-11 6:53 ` Andrea Arcangeli
@ 2011-11-11 8:42 ` Shaohua Li
0 siblings, 0 replies; 5+ messages in thread
From: Shaohua Li @ 2011-11-11 8:42 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: Andrew Morton, Hugh Dickins, linux-mm, lkml
On Fri, 2011-11-11 at 14:53 +0800, Andrea Arcangeli wrote:
> On Fri, Nov 11, 2011 at 02:36:29PM +0800, Shaohua Li wrote:
> > On Thu, 2011-11-10 at 23:36 +0800, Andrea Arcangeli wrote:
> > > On Tue, Oct 25, 2011 at 10:59:31AM +0800, Shaohua Li wrote:
> > > > Index: linux/arch/x86/include/asm/tlb.h
> > > > ===================================================================
> > > > --- linux.orig/arch/x86/include/asm/tlb.h 2011-10-25 09:00:39.000000000 +0800
> > > > +++ linux/arch/x86/include/asm/tlb.h 2011-10-25 09:02:52.000000000 +0800
> > > > @@ -4,6 +4,7 @@
> > > > #define tlb_start_vma(tlb, vma) do { } while (0)
> > > > #define tlb_end_vma(tlb, vma) do { } while (0)
> > > > #define __tlb_remove_tlb_entry(tlb, ptep, address) do { } while (0)
> > > > +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do { } while (0)
> > > > #define tlb_flush(tlb) flush_tlb_mm((tlb)->mm)
> > >
> > > This is superfluous, it's already define below as noop.
> > >
> > > >
> > > > #include <asm-generic/tlb.h>
> > > > Index: linux/include/asm-generic/tlb.h
> > > > ===================================================================
> > > > --- linux.orig/include/asm-generic/tlb.h 2011-10-25 09:00:23.000000000 +0800
> > > > +++ linux/include/asm-generic/tlb.h 2011-10-25 09:18:01.000000000 +0800
> > > > @@ -139,6 +139,16 @@ static inline void tlb_remove_page(struc
> > > > __tlb_remove_tlb_entry(tlb, ptep, address); \
> > > > } while (0)
> > > >
> > > > +#ifndef __tlb_remove_pmd_tlb_entry
> > > > +#define __tlb_remove_pmd_tlb_entry(tlb, pmdp, address) do {} while(0)
> > > > +#endif
> > > > +
> > > > +#define tlb_remove_pmd_tlb_entry(tlb, pmdp, address) \
> > > > + do { \
> > > > + tlb->need_flush = 1; \
> > > > + __tlb_remove_pmd_tlb_entry(tlb, pmdp, address); \
> > > > + } while (0)
> > >
> > > this looks weird, why do we set need_flush = 1 again, considering that
> > > we're doing tlb_remove_page() just a few lines later (which also sets
> > > tlb->need_flush = 1).
> > >
> > > Ok that other archs may need the __tlb_remove_pmd_tlb_entry to be
> > > called (and I've no idea why), but the need_flush = 1 seems
> > > unnecessary.
> > >
> > > Why other archs need the __tlb_remove_pmd_tlb_entry to be called?
> > >
> > > One way to go would be to change the tlb->need_flush = 1 in
> > > __tlb_remove_page to a VM_BUG_ON(!tlb->need_flush) and then we keep it
> > > above and we add the __tlb_remove_pmd_tlb_entry call.
> > >
> > > Or is there any place where __tlb_remove_page is called without a
> > > tlb_remove_*tlb_entry being called before it?
> > >
> > > In any case the VM_BUG_ON will verify this.
> > ok, I made the whole tlb_remove_pmd_tlb_entry() noop now. we don't need
> > add anything on it for x86 currently. We can change it later if
> > necessary.
>
> I thought it'd be cleaner to have only the __tlb_remove_*tlb_entry
> variants set need_flush=1 and have __tlb_remove_page just check that
> is set under a VM_BUG_ON. That would also avoid a second unnecessary
> need_flush = 1 for the pte case which is repeated now (it's not the
> repeated in the pmd case in your patch because it's a noop, but the
> pte case it's not a noop). Maybe it's not possible but if it's
> possible it looks better.
makes sense to me. __tlb_remove_page should always follow after
__tlb_remove_*tlb_entry. if we set need_flush=1 in
__tlb_remove_*tlb_entry, tlb_remove_*tlb_entry == __tlb_remove_*tlb
entry then, we can delete __xxx. But this sounds not related to the
issue, I'd prefer another patch to clean it up. how do you think?
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-11-11 8:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-25 2:59 [patch 3/5]thp: add tlb_remove_pmd_tlb_entry Shaohua Li
2011-11-10 15:36 ` Andrea Arcangeli
2011-11-11 6:36 ` Shaohua Li
2011-11-11 6:53 ` Andrea Arcangeli
2011-11-11 8:42 ` Shaohua Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).