Re: [patch 1/5] avoid tlb gather restarts.
On Tue, 03 Jul 2007 13:18:23 +0200 Martin Schwidefsky <[EMAIL PROTECTED]> wrote: > From: Martin Schwidefsky <[EMAIL PROTECTED]> > > If need_resched() is false in the inner loop of unmap_vmas it is > unnecessary to do a full blown tlb_finish_mmu / tlb_gather_mmu for > each ZAP_BLOCK_SIZE ptes. Do a tlb_flush_mmu() instead. That gives > architectures with a non-generic tlb flush implementation room for > optimization. The tlb_flush_mmu primitive is a available with the > generic tlb flush code, the ia64_tlb_flush_mm needs to be renamed > and a dummy function is added to arm and arm26. > > Signed-off-by: Martin Schwidefsky <[EMAIL PROTECTED]> > --- > > include/asm-arm/tlb.h |5 + > include/asm-arm26/tlb.h |5 + > include/asm-ia64/tlb.h |6 +++--- > mm/memory.c | 16 ++-- > 4 files changed, 19 insertions(+), 13 deletions(-) sparc64 broke: mm/memory.c: In function `unmap_vmas': mm/memory.c:862: error: too many arguments to function `tlb_flush_mmu' grep, please. - 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: [patch 1/5] avoid tlb gather restarts.
On Tue, 03 Jul 2007 13:18:23 +0200 Martin Schwidefsky [EMAIL PROTECTED] wrote: From: Martin Schwidefsky [EMAIL PROTECTED] If need_resched() is false in the inner loop of unmap_vmas it is unnecessary to do a full blown tlb_finish_mmu / tlb_gather_mmu for each ZAP_BLOCK_SIZE ptes. Do a tlb_flush_mmu() instead. That gives architectures with a non-generic tlb flush implementation room for optimization. The tlb_flush_mmu primitive is a available with the generic tlb flush code, the ia64_tlb_flush_mm needs to be renamed and a dummy function is added to arm and arm26. Signed-off-by: Martin Schwidefsky [EMAIL PROTECTED] --- include/asm-arm/tlb.h |5 + include/asm-arm26/tlb.h |5 + include/asm-ia64/tlb.h |6 +++--- mm/memory.c | 16 ++-- 4 files changed, 19 insertions(+), 13 deletions(-) sparc64 broke: mm/memory.c: In function `unmap_vmas': mm/memory.c:862: error: too many arguments to function `tlb_flush_mmu' grep, please. - 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: [patch 1/5] avoid tlb gather restarts.
On Tue, 2007-07-03 at 18:42 +0100, Hugh Dickins wrote: > > If need_resched() is false in the inner loop of unmap_vmas it is > > unnecessary to do a full blown tlb_finish_mmu / tlb_gather_mmu for > > each ZAP_BLOCK_SIZE ptes. Do a tlb_flush_mmu() instead. That gives > > architectures with a non-generic tlb flush implementation room for > > optimization. The tlb_flush_mmu primitive is a available with the > > generic tlb flush code, the ia64_tlb_flush_mm needs to be renamed > > and a dummy function is added to arm and arm26. > > > > Signed-off-by: Martin Schwidefsky <[EMAIL PROTECTED]> > > Acked-by: Hugh Dickins <[EMAIL PROTECTED]> > > (Looking at it, I see that we could argue that there ought to be a > need_resched() etc. check after your tlb_flush_mmu() in unmap_vmas, > in case it's spent a long while in there on some arches; but I don't > think we have the ZAP_BLOCK_SIZE tuned with any great precision, and > you'd at worst be doubling the latency there, so let's not worry > about it. I write this merely in order to reserve myself an > "I told you so" if anyone ever notices increased latency ;) Hmm, we'd have to repeat the longish if statement to make sure we don't miss a cond_resched after tlb_flush_mmu. I'd rather not do that. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. - 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: [patch 1/5] avoid tlb gather restarts.
On Tue, 2007-07-03 at 18:42 +0100, Hugh Dickins wrote: If need_resched() is false in the inner loop of unmap_vmas it is unnecessary to do a full blown tlb_finish_mmu / tlb_gather_mmu for each ZAP_BLOCK_SIZE ptes. Do a tlb_flush_mmu() instead. That gives architectures with a non-generic tlb flush implementation room for optimization. The tlb_flush_mmu primitive is a available with the generic tlb flush code, the ia64_tlb_flush_mm needs to be renamed and a dummy function is added to arm and arm26. Signed-off-by: Martin Schwidefsky [EMAIL PROTECTED] Acked-by: Hugh Dickins [EMAIL PROTECTED] (Looking at it, I see that we could argue that there ought to be a need_resched() etc. check after your tlb_flush_mmu() in unmap_vmas, in case it's spent a long while in there on some arches; but I don't think we have the ZAP_BLOCK_SIZE tuned with any great precision, and you'd at worst be doubling the latency there, so let's not worry about it. I write this merely in order to reserve myself an I told you so if anyone ever notices increased latency ;) Hmm, we'd have to repeat the longish if statement to make sure we don't miss a cond_resched after tlb_flush_mmu. I'd rather not do that. -- blue skies, Martin. Reality continues to ruin my life. - Calvin. - 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: [patch 1/5] avoid tlb gather restarts.
On Tue, 3 Jul 2007, Martin Schwidefsky wrote:
> From: Martin Schwidefsky <[EMAIL PROTECTED]>
>
> If need_resched() is false in the inner loop of unmap_vmas it is
> unnecessary to do a full blown tlb_finish_mmu / tlb_gather_mmu for
> each ZAP_BLOCK_SIZE ptes. Do a tlb_flush_mmu() instead. That gives
> architectures with a non-generic tlb flush implementation room for
> optimization. The tlb_flush_mmu primitive is a available with the
> generic tlb flush code, the ia64_tlb_flush_mm needs to be renamed
> and a dummy function is added to arm and arm26.
>
> Signed-off-by: Martin Schwidefsky <[EMAIL PROTECTED]>
Acked-by: Hugh Dickins <[EMAIL PROTECTED]>
(Looking at it, I see that we could argue that there ought to be a
need_resched() etc. check after your tlb_flush_mmu() in unmap_vmas,
in case it's spent a long while in there on some arches; but I don't
think we have the ZAP_BLOCK_SIZE tuned with any great precision, and
you'd at worst be doubling the latency there, so let's not worry
about it. I write this merely in order to reserve myself an
"I told you so" if anyone ever notices increased latency ;)
> ---
>
> include/asm-arm/tlb.h |5 +
> include/asm-arm26/tlb.h |5 +
> include/asm-ia64/tlb.h |6 +++---
> mm/memory.c | 16 ++--
> 4 files changed, 19 insertions(+), 13 deletions(-)
>
> diff -urpN linux-2.6/include/asm-arm/tlb.h
> linux-2.6-patched/include/asm-arm/tlb.h
> --- linux-2.6/include/asm-arm/tlb.h 2006-11-08 10:45:43.0 +0100
> +++ linux-2.6-patched/include/asm-arm/tlb.h 2007-07-03 12:56:46.0
> +0200
> @@ -52,6 +52,11 @@ tlb_gather_mmu(struct mm_struct *mm, uns
> }
>
> static inline void
> +tlb_flush_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
> +{
> +}
> +
> +static inline void
> tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long
> end)
> {
> if (tlb->fullmm)
> diff -urpN linux-2.6/include/asm-arm26/tlb.h
> linux-2.6-patched/include/asm-arm26/tlb.h
> --- linux-2.6/include/asm-arm26/tlb.h 2006-11-08 10:45:43.0 +0100
> +++ linux-2.6-patched/include/asm-arm26/tlb.h 2007-07-03 12:56:46.0
> +0200
> @@ -29,6 +29,11 @@ tlb_gather_mmu(struct mm_struct *mm, uns
> }
>
> static inline void
> +tlb_flush_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
> +{
> +}
> +
> +static inline void
> tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long
> end)
> {
> if (tlb->need_flush)
> diff -urpN linux-2.6/include/asm-ia64/tlb.h
> linux-2.6-patched/include/asm-ia64/tlb.h
> --- linux-2.6/include/asm-ia64/tlb.h 2006-11-08 10:45:45.0 +0100
> +++ linux-2.6-patched/include/asm-ia64/tlb.h 2007-07-03 12:56:46.0
> +0200
> @@ -72,7 +72,7 @@ DECLARE_PER_CPU(struct mmu_gather, mmu_g
> * freed pages that where gathered up to this point.
> */
> static inline void
> -ia64_tlb_flush_mmu (struct mmu_gather *tlb, unsigned long start, unsigned
> long end)
> +tlb_flush_mmu (struct mmu_gather *tlb, unsigned long start, unsigned long
> end)
> {
> unsigned int nr;
>
> @@ -160,7 +160,7 @@ tlb_finish_mmu (struct mmu_gather *tlb,
>* Note: tlb->nr may be 0 at this point, so we can't rely on
> tlb->start_addr and
>* tlb->end_addr.
>*/
> - ia64_tlb_flush_mmu(tlb, start, end);
> + tlb_flush_mmu(tlb, start, end);
>
> /* keep the page table cache within bounds */
> check_pgt_cache();
> @@ -184,7 +184,7 @@ tlb_remove_page (struct mmu_gather *tlb,
> }
> tlb->pages[tlb->nr++] = page;
> if (tlb->nr >= FREE_PTE_NR)
> - ia64_tlb_flush_mmu(tlb, tlb->start_addr, tlb->end_addr);
> + tlb_flush_mmu(tlb, tlb->start_addr, tlb->end_addr);
> }
>
> /*
> diff -urpN linux-2.6/mm/memory.c linux-2.6-patched/mm/memory.c
> --- linux-2.6/mm/memory.c 2007-06-18 09:43:22.0 +0200
> +++ linux-2.6-patched/mm/memory.c 2007-07-03 12:56:46.0 +0200
> @@ -853,18 +853,15 @@ unsigned long unmap_vmas(struct mmu_gath
> break;
> }
>
> - tlb_finish_mmu(*tlbp, tlb_start, start);
> -
> if (need_resched() ||
> (i_mmap_lock && need_lockbreak(i_mmap_lock))) {
> - if (i_mmap_lock) {
> - *tlbp = NULL;
> + if (i_mmap_lock)
> goto out;
> - }
> + tlb_finish_mmu(*tlbp, tlb_start, start);
> cond_resched();
> - }
> -
> - *tlbp = tlb_gather_mmu(vma->vm_mm, fullmm);
> + *tlbp = tlb_gather_mmu(vma->vm_mm, fullmm);
> + } else
> + tlb_flush_mmu(*tlbp, tlb_start, start);
>
Re: [patch 1/5] avoid tlb gather restarts.
On Tue, 3 Jul 2007, Martin Schwidefsky wrote:
From: Martin Schwidefsky [EMAIL PROTECTED]
If need_resched() is false in the inner loop of unmap_vmas it is
unnecessary to do a full blown tlb_finish_mmu / tlb_gather_mmu for
each ZAP_BLOCK_SIZE ptes. Do a tlb_flush_mmu() instead. That gives
architectures with a non-generic tlb flush implementation room for
optimization. The tlb_flush_mmu primitive is a available with the
generic tlb flush code, the ia64_tlb_flush_mm needs to be renamed
and a dummy function is added to arm and arm26.
Signed-off-by: Martin Schwidefsky [EMAIL PROTECTED]
Acked-by: Hugh Dickins [EMAIL PROTECTED]
(Looking at it, I see that we could argue that there ought to be a
need_resched() etc. check after your tlb_flush_mmu() in unmap_vmas,
in case it's spent a long while in there on some arches; but I don't
think we have the ZAP_BLOCK_SIZE tuned with any great precision, and
you'd at worst be doubling the latency there, so let's not worry
about it. I write this merely in order to reserve myself an
I told you so if anyone ever notices increased latency ;)
---
include/asm-arm/tlb.h |5 +
include/asm-arm26/tlb.h |5 +
include/asm-ia64/tlb.h |6 +++---
mm/memory.c | 16 ++--
4 files changed, 19 insertions(+), 13 deletions(-)
diff -urpN linux-2.6/include/asm-arm/tlb.h
linux-2.6-patched/include/asm-arm/tlb.h
--- linux-2.6/include/asm-arm/tlb.h 2006-11-08 10:45:43.0 +0100
+++ linux-2.6-patched/include/asm-arm/tlb.h 2007-07-03 12:56:46.0
+0200
@@ -52,6 +52,11 @@ tlb_gather_mmu(struct mm_struct *mm, uns
}
static inline void
+tlb_flush_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
+{
+}
+
+static inline void
tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long
end)
{
if (tlb-fullmm)
diff -urpN linux-2.6/include/asm-arm26/tlb.h
linux-2.6-patched/include/asm-arm26/tlb.h
--- linux-2.6/include/asm-arm26/tlb.h 2006-11-08 10:45:43.0 +0100
+++ linux-2.6-patched/include/asm-arm26/tlb.h 2007-07-03 12:56:46.0
+0200
@@ -29,6 +29,11 @@ tlb_gather_mmu(struct mm_struct *mm, uns
}
static inline void
+tlb_flush_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
+{
+}
+
+static inline void
tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long
end)
{
if (tlb-need_flush)
diff -urpN linux-2.6/include/asm-ia64/tlb.h
linux-2.6-patched/include/asm-ia64/tlb.h
--- linux-2.6/include/asm-ia64/tlb.h 2006-11-08 10:45:45.0 +0100
+++ linux-2.6-patched/include/asm-ia64/tlb.h 2007-07-03 12:56:46.0
+0200
@@ -72,7 +72,7 @@ DECLARE_PER_CPU(struct mmu_gather, mmu_g
* freed pages that where gathered up to this point.
*/
static inline void
-ia64_tlb_flush_mmu (struct mmu_gather *tlb, unsigned long start, unsigned
long end)
+tlb_flush_mmu (struct mmu_gather *tlb, unsigned long start, unsigned long
end)
{
unsigned int nr;
@@ -160,7 +160,7 @@ tlb_finish_mmu (struct mmu_gather *tlb,
* Note: tlb-nr may be 0 at this point, so we can't rely on
tlb-start_addr and
* tlb-end_addr.
*/
- ia64_tlb_flush_mmu(tlb, start, end);
+ tlb_flush_mmu(tlb, start, end);
/* keep the page table cache within bounds */
check_pgt_cache();
@@ -184,7 +184,7 @@ tlb_remove_page (struct mmu_gather *tlb,
}
tlb-pages[tlb-nr++] = page;
if (tlb-nr = FREE_PTE_NR)
- ia64_tlb_flush_mmu(tlb, tlb-start_addr, tlb-end_addr);
+ tlb_flush_mmu(tlb, tlb-start_addr, tlb-end_addr);
}
/*
diff -urpN linux-2.6/mm/memory.c linux-2.6-patched/mm/memory.c
--- linux-2.6/mm/memory.c 2007-06-18 09:43:22.0 +0200
+++ linux-2.6-patched/mm/memory.c 2007-07-03 12:56:46.0 +0200
@@ -853,18 +853,15 @@ unsigned long unmap_vmas(struct mmu_gath
break;
}
- tlb_finish_mmu(*tlbp, tlb_start, start);
-
if (need_resched() ||
(i_mmap_lock need_lockbreak(i_mmap_lock))) {
- if (i_mmap_lock) {
- *tlbp = NULL;
+ if (i_mmap_lock)
goto out;
- }
+ tlb_finish_mmu(*tlbp, tlb_start, start);
cond_resched();
- }
-
- *tlbp = tlb_gather_mmu(vma-vm_mm, fullmm);
+ *tlbp = tlb_gather_mmu(vma-vm_mm, fullmm);
+ } else
+ tlb_flush_mmu(*tlbp, tlb_start, start);
tlb_start_valid = 0;
zap_work = ZAP_BLOCK_SIZE;
}
@@ -892,8 +889,7 @@ unsigned long zap_page_range(struct
Re: [patch 1/5] avoid tlb gather restarts.
On Fri, 29 Jun 2007, Martin Schwidefsky wrote: > On Fri, 2007-06-29 at 19:56 +0100, Hugh Dickins wrote: > > I don't dare comment on your page_mkclean_one patch (5/5), > > that dirty page business has grown too subtle for me. > > Oh yes, the dirty handling is tricky I'll move that discussion over to 5/5 and Cc Peter (sorry I was too lazy to do so in the first place). > > On Fri, 29 Jun 2007, Martin Schwidefsky wrote: > > You think you're just moving the finish/gather to where they're > > actually necessary; but the thing is, that per-cpu struct mmu_gather > > is liable to accumulate a lot of unpreemptible work for the future > > tlb_finish_mmu, particularly when anon pages are associated with swap. > > Hmm, ok, so you are saying that we should do a flush at the end of each > vma. I think of it as doing a flush every ZAP_BLOCK_SIZE, with the imperfect structure of the loop forcing perhaps an early flush at the end of each vma: I seem to assume large vmas, and you to assume small ones. IIRC, the common case for doing multiple vmas here is exit, when it ends up that the TLB flush can often be skipped because already done by the switch from exiting task; so the premature flush per vma doesn't matter much. But treat that claim with maximum scepticism: I've not rechecked it, several aspects may be wrong. What I do remember is that (at least on i386) there's a lot less actual TLB flushing done here than it appears from the outside. > > So although there may be no need to resched right now, if we keep on > > gathering more and more without flushing, we'll be very unresponsive > > when a resched is needed later on. Hence Ingo's ZAP_BLOCK_SIZE to > > split it up, small when CONFIG_PREEMPT, more reasonable but still > > limited when not. > > Would it be acceptable to call tlb_flush_mmu instead of the > tlb_finish_mmu / tlb_gather_mmu pair if the condition around > cond_resched evaluates to false? That sounds a good idea, yes, that should be fine. But beware, tlb_flush_mmu is an internal detail of the asm-generic/tlb.h method and perhaps some others, it currently doesn't exist on some arches. I think you just need to add a simple one to arm & arm26, and take the "ia64_" off the ia64 one. powerpc and sparc64 go about it all a bit differently, but it should be easy to give them one too. There may be some others missing. > The background for this change is that I'm working on another patch that > will change the tlb flushing for s390 quite a bit. We won't have > anything to flush with tlb_finish_mmu because we will either flush all > tlbs with tlb_gather_mmu or each pte seperatly. The pages will always be > freed immediatly. If we are forced to restart the tlb gather then we'll > do multiple flush_tlb_mm because the information that we already flushed > everything is lost with tlb_finish_mmu. Thanks for the info. Sounds like we may have trouble ahead when rearranging this stuff, easy to forget s390 from our assumptions: keep watch! Hugh - 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: [patch 1/5] avoid tlb gather restarts.
On Fri, 29 Jun 2007, Martin Schwidefsky wrote: On Fri, 2007-06-29 at 19:56 +0100, Hugh Dickins wrote: I don't dare comment on your page_mkclean_one patch (5/5), that dirty page business has grown too subtle for me. Oh yes, the dirty handling is tricky I'll move that discussion over to 5/5 and Cc Peter (sorry I was too lazy to do so in the first place). On Fri, 29 Jun 2007, Martin Schwidefsky wrote: You think you're just moving the finish/gather to where they're actually necessary; but the thing is, that per-cpu struct mmu_gather is liable to accumulate a lot of unpreemptible work for the future tlb_finish_mmu, particularly when anon pages are associated with swap. Hmm, ok, so you are saying that we should do a flush at the end of each vma. I think of it as doing a flush every ZAP_BLOCK_SIZE, with the imperfect structure of the loop forcing perhaps an early flush at the end of each vma: I seem to assume large vmas, and you to assume small ones. IIRC, the common case for doing multiple vmas here is exit, when it ends up that the TLB flush can often be skipped because already done by the switch from exiting task; so the premature flush per vma doesn't matter much. But treat that claim with maximum scepticism: I've not rechecked it, several aspects may be wrong. What I do remember is that (at least on i386) there's a lot less actual TLB flushing done here than it appears from the outside. So although there may be no need to resched right now, if we keep on gathering more and more without flushing, we'll be very unresponsive when a resched is needed later on. Hence Ingo's ZAP_BLOCK_SIZE to split it up, small when CONFIG_PREEMPT, more reasonable but still limited when not. Would it be acceptable to call tlb_flush_mmu instead of the tlb_finish_mmu / tlb_gather_mmu pair if the condition around cond_resched evaluates to false? That sounds a good idea, yes, that should be fine. But beware, tlb_flush_mmu is an internal detail of the asm-generic/tlb.h method and perhaps some others, it currently doesn't exist on some arches. I think you just need to add a simple one to arm arm26, and take the ia64_ off the ia64 one. powerpc and sparc64 go about it all a bit differently, but it should be easy to give them one too. There may be some others missing. The background for this change is that I'm working on another patch that will change the tlb flushing for s390 quite a bit. We won't have anything to flush with tlb_finish_mmu because we will either flush all tlbs with tlb_gather_mmu or each pte seperatly. The pages will always be freed immediatly. If we are forced to restart the tlb gather then we'll do multiple flush_tlb_mm because the information that we already flushed everything is lost with tlb_finish_mmu. Thanks for the info. Sounds like we may have trouble ahead when rearranging this stuff, easy to forget s390 from our assumptions: keep watch! Hugh - 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: [patch 1/5] avoid tlb gather restarts.
On Fri, 2007-06-29 at 19:56 +0100, Hugh Dickins wrote: > I don't dare comment on your page_mkclean_one patch (5/5), > that dirty page business has grown too subtle for me. Oh yes, the dirty handling is tricky. I had to fix a really nasty bug with it lately. As for page_mkclean_one the difference is that it doesn't claim a page is dirty if only the write protect bit has not been set. If we manage to lose dirty bits from ptes and have to rely on the write protect bit to take over the job, then we have a different problem altogether, no ? > Your cleanups 2-4 look good, especially the mm_types.h one (how > confident are you that everything builds?), and I'm glad we can > now lay ptep_establish to rest. Though I think you may have > missed removing a __HAVE_ARCH_PTEP... from frv at least? Ok, thanks for the review. I take a look at frv to see if I missed something. > But this one... > > On Fri, 29 Jun 2007, Martin Schwidefsky wrote: > > > If need_resched() is false it is unnecessary to call tlb_finish_mmu() > > and tlb_gather_mmu() for each vma in unmap_vmas(). Moving the tlb gather > > restart under the if that contains the cond_resched() will avoid > > unnecessary tlb flush operations that are triggered by tlb_finish_mmu() > > and tlb_gather_mmu(). > > > > Signed-off-by: Martin Schwidefsky <[EMAIL PROTECTED]> > > Sorry, no. It looks reasonable, but unmap_vmas is treading a delicate > and uncomfortable line between hi-performance and lo-latency: you've > chosen to improve performance at the expense of latency. That it true, my only concern had been performance. You likely have a point here. > You think you're just moving the finish/gather to where they're > actually necessary; but the thing is, that per-cpu struct mmu_gather > is liable to accumulate a lot of unpreemptible work for the future > tlb_finish_mmu, particularly when anon pages are associated with swap. Hmm, ok, so you are saying that we should do a flush at the end of each vma. > So although there may be no need to resched right now, if we keep on > gathering more and more without flushing, we'll be very unresponsive > when a resched is needed later on. Hence Ingo's ZAP_BLOCK_SIZE to > split it up, small when CONFIG_PREEMPT, more reasonable but still > limited when not. Would it be acceptable to call tlb_flush_mmu instead of the tlb_finish_mmu / tlb_gather_mmu pair if the condition around cond_resched evaluates to false? The background for this change is that I'm working on another patch that will change the tlb flushing for s390 quite a bit. We won't have anything to flush with tlb_finish_mmu because we will either flush all tlbs with tlb_gather_mmu or each pte seperatly. The pages will always be freed immediatly. If we are forced to restart the tlb gather then we'll do multiple flush_tlb_mm because the information that we already flushed everything is lost with tlb_finish_mmu. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. - 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: [patch 1/5] avoid tlb gather restarts.
I don't dare comment on your page_mkclean_one patch (5/5),
that dirty page business has grown too subtle for me.
Your cleanups 2-4 look good, especially the mm_types.h one (how
confident are you that everything builds?), and I'm glad we can
now lay ptep_establish to rest. Though I think you may have
missed removing a __HAVE_ARCH_PTEP... from frv at least?
But this one...
On Fri, 29 Jun 2007, Martin Schwidefsky wrote:
> If need_resched() is false it is unnecessary to call tlb_finish_mmu()
> and tlb_gather_mmu() for each vma in unmap_vmas(). Moving the tlb gather
> restart under the if that contains the cond_resched() will avoid
> unnecessary tlb flush operations that are triggered by tlb_finish_mmu()
> and tlb_gather_mmu().
>
> Signed-off-by: Martin Schwidefsky <[EMAIL PROTECTED]>
Sorry, no. It looks reasonable, but unmap_vmas is treading a delicate
and uncomfortable line between hi-performance and lo-latency: you've
chosen to improve performance at the expense of latency.
You think you're just moving the finish/gather to where they're
actually necessary; but the thing is, that per-cpu struct mmu_gather
is liable to accumulate a lot of unpreemptible work for the future
tlb_finish_mmu, particularly when anon pages are associated with swap.
So although there may be no need to resched right now, if we keep on
gathering more and more without flushing, we'll be very unresponsive
when a resched is needed later on. Hence Ingo's ZAP_BLOCK_SIZE to
split it up, small when CONFIG_PREEMPT, more reasonable but still
limited when not.
I expect there is some tinkering which could be done to improve it a
little; but my ambition has always been to eliminate ZAP_BLOCK_SIZE,
get away from the per-cpu'ness of the mmu_gather, and make unmap_vmas
preemptible. But the i_mmap_lock case, and the per-arch variations
in TLB flushing, have forever stalled me.
Hugh
> ---
>
> mm/memory.c |7 +++
> 1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff -urpN linux-2.6/mm/memory.c linux-2.6-patched/mm/memory.c
> --- linux-2.6/mm/memory.c 2007-06-29 15:44:08.0 +0200
> +++ linux-2.6-patched/mm/memory.c 2007-06-29 15:44:08.0 +0200
> @@ -851,19 +851,18 @@ unsigned long unmap_vmas(struct mmu_gath
> break;
> }
>
> - tlb_finish_mmu(*tlbp, tlb_start, start);
> -
> if (need_resched() ||
> (i_mmap_lock && need_lockbreak(i_mmap_lock))) {
> + tlb_finish_mmu(*tlbp, tlb_start, start);
> if (i_mmap_lock) {
> *tlbp = NULL;
> goto out;
> }
> cond_resched();
> + *tlbp = tlb_gather_mmu(vma->vm_mm, fullmm);
> + tlb_start_valid = 0;
> }
>
> - *tlbp = tlb_gather_mmu(vma->vm_mm, fullmm);
> - tlb_start_valid = 0;
> zap_work = ZAP_BLOCK_SIZE;
> }
> }
-
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: [patch 1/5] avoid tlb gather restarts.
I don't dare comment on your page_mkclean_one patch (5/5),
that dirty page business has grown too subtle for me.
Your cleanups 2-4 look good, especially the mm_types.h one (how
confident are you that everything builds?), and I'm glad we can
now lay ptep_establish to rest. Though I think you may have
missed removing a __HAVE_ARCH_PTEP... from frv at least?
But this one...
On Fri, 29 Jun 2007, Martin Schwidefsky wrote:
If need_resched() is false it is unnecessary to call tlb_finish_mmu()
and tlb_gather_mmu() for each vma in unmap_vmas(). Moving the tlb gather
restart under the if that contains the cond_resched() will avoid
unnecessary tlb flush operations that are triggered by tlb_finish_mmu()
and tlb_gather_mmu().
Signed-off-by: Martin Schwidefsky [EMAIL PROTECTED]
Sorry, no. It looks reasonable, but unmap_vmas is treading a delicate
and uncomfortable line between hi-performance and lo-latency: you've
chosen to improve performance at the expense of latency.
You think you're just moving the finish/gather to where they're
actually necessary; but the thing is, that per-cpu struct mmu_gather
is liable to accumulate a lot of unpreemptible work for the future
tlb_finish_mmu, particularly when anon pages are associated with swap.
So although there may be no need to resched right now, if we keep on
gathering more and more without flushing, we'll be very unresponsive
when a resched is needed later on. Hence Ingo's ZAP_BLOCK_SIZE to
split it up, small when CONFIG_PREEMPT, more reasonable but still
limited when not.
I expect there is some tinkering which could be done to improve it a
little; but my ambition has always been to eliminate ZAP_BLOCK_SIZE,
get away from the per-cpu'ness of the mmu_gather, and make unmap_vmas
preemptible. But the i_mmap_lock case, and the per-arch variations
in TLB flushing, have forever stalled me.
Hugh
---
mm/memory.c |7 +++
1 files changed, 3 insertions(+), 4 deletions(-)
diff -urpN linux-2.6/mm/memory.c linux-2.6-patched/mm/memory.c
--- linux-2.6/mm/memory.c 2007-06-29 15:44:08.0 +0200
+++ linux-2.6-patched/mm/memory.c 2007-06-29 15:44:08.0 +0200
@@ -851,19 +851,18 @@ unsigned long unmap_vmas(struct mmu_gath
break;
}
- tlb_finish_mmu(*tlbp, tlb_start, start);
-
if (need_resched() ||
(i_mmap_lock need_lockbreak(i_mmap_lock))) {
+ tlb_finish_mmu(*tlbp, tlb_start, start);
if (i_mmap_lock) {
*tlbp = NULL;
goto out;
}
cond_resched();
+ *tlbp = tlb_gather_mmu(vma-vm_mm, fullmm);
+ tlb_start_valid = 0;
}
- *tlbp = tlb_gather_mmu(vma-vm_mm, fullmm);
- tlb_start_valid = 0;
zap_work = ZAP_BLOCK_SIZE;
}
}
-
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: [patch 1/5] avoid tlb gather restarts.
On Fri, 2007-06-29 at 19:56 +0100, Hugh Dickins wrote: I don't dare comment on your page_mkclean_one patch (5/5), that dirty page business has grown too subtle for me. Oh yes, the dirty handling is tricky. I had to fix a really nasty bug with it lately. As for page_mkclean_one the difference is that it doesn't claim a page is dirty if only the write protect bit has not been set. If we manage to lose dirty bits from ptes and have to rely on the write protect bit to take over the job, then we have a different problem altogether, no ? Your cleanups 2-4 look good, especially the mm_types.h one (how confident are you that everything builds?), and I'm glad we can now lay ptep_establish to rest. Though I think you may have missed removing a __HAVE_ARCH_PTEP... from frv at least? Ok, thanks for the review. I take a look at frv to see if I missed something. But this one... On Fri, 29 Jun 2007, Martin Schwidefsky wrote: If need_resched() is false it is unnecessary to call tlb_finish_mmu() and tlb_gather_mmu() for each vma in unmap_vmas(). Moving the tlb gather restart under the if that contains the cond_resched() will avoid unnecessary tlb flush operations that are triggered by tlb_finish_mmu() and tlb_gather_mmu(). Signed-off-by: Martin Schwidefsky [EMAIL PROTECTED] Sorry, no. It looks reasonable, but unmap_vmas is treading a delicate and uncomfortable line between hi-performance and lo-latency: you've chosen to improve performance at the expense of latency. That it true, my only concern had been performance. You likely have a point here. You think you're just moving the finish/gather to where they're actually necessary; but the thing is, that per-cpu struct mmu_gather is liable to accumulate a lot of unpreemptible work for the future tlb_finish_mmu, particularly when anon pages are associated with swap. Hmm, ok, so you are saying that we should do a flush at the end of each vma. So although there may be no need to resched right now, if we keep on gathering more and more without flushing, we'll be very unresponsive when a resched is needed later on. Hence Ingo's ZAP_BLOCK_SIZE to split it up, small when CONFIG_PREEMPT, more reasonable but still limited when not. Would it be acceptable to call tlb_flush_mmu instead of the tlb_finish_mmu / tlb_gather_mmu pair if the condition around cond_resched evaluates to false? The background for this change is that I'm working on another patch that will change the tlb flushing for s390 quite a bit. We won't have anything to flush with tlb_finish_mmu because we will either flush all tlbs with tlb_gather_mmu or each pte seperatly. The pages will always be freed immediatly. If we are forced to restart the tlb gather then we'll do multiple flush_tlb_mm because the information that we already flushed everything is lost with tlb_finish_mmu. -- blue skies, Martin. Reality continues to ruin my life. - Calvin. - 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/
