| Message ID | 20180612071621.26775-4-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 12, 2018 at 12:16 AM Nicholas Piggin <npiggin@gmail.com> wrote: > > This brings the number of tlbiel instructions required by a kernel > compile from 33M to 25M, most avoided from exec->shift_arg_pages. And this shows that "page_start/end" is purely for powerpc and used nowhere else. The previous patch should have been to purely powerpc page table walking and not touch asm-generic/tlb.h I think you should make those changes to arch/powerpc/include/asm/tlb.h. If that means you can't use the generic header, then so be it. Or maybe you can embed the generic case in some ppc-specific structures, and use 90% of the generic code just with your added wrappers for that radix invalidation on top. But don't make other architectures do pointless work that doesn't matter - or make sense - for them. Linus
On Tue, 12 Jun 2018 11:18:27 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jun 12, 2018 at 12:16 AM Nicholas Piggin <npiggin@gmail.com> wrote: > > > > This brings the number of tlbiel instructions required by a kernel > > compile from 33M to 25M, most avoided from exec->shift_arg_pages. > > And this shows that "page_start/end" is purely for powerpc and used > nowhere else. > > The previous patch should have been to purely powerpc page table > walking and not touch asm-generic/tlb.h > > I think you should make those changes to > arch/powerpc/include/asm/tlb.h. If that means you can't use the > generic header, then so be it. I can make it ppc specific if nobody else would use it. But at least mmu notifiers AFAIKS would rather use a precise range. > Or maybe you can embed the generic case in some ppc-specific > structures, and use 90% of the generic code just with your added > wrappers for that radix invalidation on top. Would you mind another arch specific ifdefs in there? > > But don't make other architectures do pointless work that doesn't > matter - or make sense - for them. Okay sure, and this is the reason for the wide cc list. Intel does need it of course, from 4.10.3.1 of the dev manual: — The processor may create a PML4-cache entry even if there are no translations for any linear address that might use that entry (e.g., because the P flags are 0 in all entries in the referenced page-directory-pointer table). But I'm sure others would not have paging structure caches at all (some don't even walk the page tables in hardware right?). Maybe they're all doing their own thing though. Thanks, Nick
On Tue, Jun 12, 2018 at 3:31 PM Nicholas Piggin <npiggin@gmail.com> wrote: > > Okay sure, and this is the reason for the wide cc list. Intel does > need it of course, from 4.10.3.1 of the dev manual: > > — The processor may create a PML4-cache entry even if there are no > translations for any linear address that might use that entry > (e.g., because the P flags are 0 in all entries in the referenced > page-directory-pointer table). But does intel need it? Because I don't see it. We already do the __tlb_adjust_range(), and we never tear down the highest-level page tables afaik. Am I missing something? Linus
On Tue, 12 Jun 2018 15:42:34 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jun 12, 2018 at 3:31 PM Nicholas Piggin <npiggin@gmail.com> wrote: > > > > Okay sure, and this is the reason for the wide cc list. Intel does > > need it of course, from 4.10.3.1 of the dev manual: > > > > — The processor may create a PML4-cache entry even if there are no > > translations for any linear address that might use that entry > > (e.g., because the P flags are 0 in all entries in the referenced > > page-directory-pointer table). > > But does intel need it? > > Because I don't see it. We already do the __tlb_adjust_range(), and we > never tear down the highest-level page tables afaik. > > Am I missing something? Sorry I mean Intel needs the existing behaviour of range flush expanded to cover page table pages.... right? The manual has similar wording for lower levels of page tables too. So it does need to send an invalidate *somewhere* that a freed page table page covers, even if no valid pte was torn down. Thanks, Nick
On Tue, Jun 12, 2018 at 4:09 PM Nicholas Piggin <npiggin@gmail.com> wrote: > > Sorry I mean Intel needs the existing behaviour of range flush expanded > to cover page table pages.... right? Right. Intel depends on the current thing, ie if a page table *itself* is freed, we will will need to do a flush, but it's the exact same flush as if there had been a regular page there. That's already handled by (for example) pud_free_tlb() doing the __tlb_adjust_range(). Again, I may be missing entirely what you're talking about, because it feels like we're talking across each other. My argument is that your new patches in (2-3 in the series - patch #1 looks ok) seem to be fundamentally specific to things that have a *different* tlb invalidation for the directory entries than for the leaf entries. But that's not what at least x86 has, and not what the generic code has done. I think it might be fine to introduce a few new helpers that end up being no-ops for the traditional cases. I just don't think it makes sense to maintain a set of range values that then aren't actually used in the general case. Linus
On Tue, Jun 12, 2018 at 4:26 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Right. Intel depends on the current thing, ie if a page table > *itself* is freed, we will will need to do a flush, but it's the exact > same flush as if there had been a regular page there. > > That's already handled by (for example) pud_free_tlb() doing the > __tlb_adjust_range(). Side note: I guess we _could_ make the "page directory" flush be special on x86 too. Right now a page directory flush just counts as a range, and then a range that is more that a few entries just means "flush everything". End result: in practice, every time you free a page directory, you flush the whole TLB because it looks identical to flushing a large range of pages. And in _theory_, maybe you could have just used "invalpg" with a targeted address instead. In fact, I think a single invlpg invalidates _all_ caches for the associated MM, but don't quote me on that. That said, I don't think this is a common case. But I think that *if* you extend this to be aware of the page directory caches, and _if_ you extend it to cover both ppc and x86, at that point all my "this isn't generic" arguments go away. Because once x86 does it, it's "common enough" that it counts as generic. It may be only a single other architecture, but it's the bulk of all the development machines, so.. Linus
On Tue, 12 Jun 2018 16:26:33 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jun 12, 2018 at 4:09 PM Nicholas Piggin <npiggin@gmail.com> wrote: > > > > Sorry I mean Intel needs the existing behaviour of range flush expanded > > to cover page table pages.... right? > > Right. Intel depends on the current thing, ie if a page table > *itself* is freed, we will will need to do a flush, but it's the exact > same flush as if there had been a regular page there. > > That's already handled by (for example) pud_free_tlb() doing the > __tlb_adjust_range(). Agreed. > > Again, I may be missing entirely what you're talking about, because it > feels like we're talking across each other. > > My argument is that your new patches in (2-3 in the series - patch #1 > looks ok) seem to be fundamentally specific to things that have a > *different* tlb invalidation for the directory entries than for the > leaf entries. Yes I think I confused myself a bit. You're right these patches are only useful if there is no page structure cache, or if it's managed separately from TLB invalidation. > > But that's not what at least x86 has, and not what the generic code has done. > > I think it might be fine to introduce a few new helpers that end up > being no-ops for the traditional cases. > > I just don't think it makes sense to maintain a set of range values > that then aren't actually used in the general case. Sure, I'll make it optional. That would probably give a better result for powerpc too because it doesn't need to maintain two ranges either. Thanks, Nick
On Tue, 12 Jun 2018 16:39:55 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jun 12, 2018 at 4:26 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Right. Intel depends on the current thing, ie if a page table > > *itself* is freed, we will will need to do a flush, but it's the exact > > same flush as if there had been a regular page there. > > > > That's already handled by (for example) pud_free_tlb() doing the > > __tlb_adjust_range(). > > Side note: I guess we _could_ make the "page directory" flush be > special on x86 too. > > Right now a page directory flush just counts as a range, and then a > range that is more that a few entries just means "flush everything". > > End result: in practice, every time you free a page directory, you > flush the whole TLB because it looks identical to flushing a large > range of pages. > > And in _theory_, maybe you could have just used "invalpg" with a > targeted address instead. In fact, I think a single invlpg invalidates > _all_ caches for the associated MM, but don't quote me on that. Yeah I was thinking that, you could treat it separately (similar to powerpc maybe) despite using the same instructions to invalidate it. > That said, I don't think this is a common case. But I think that *if* > you extend this to be aware of the page directory caches, and _if_ you > extend it to cover both ppc and x86, at that point all my "this isn't > generic" arguments go away. > > Because once x86 does it, it's "common enough" that it counts as > generic. It may be only a single other architecture, but it's the bulk > of all the development machines, so.. I'll do the small step first (basically just this patch as an opt-in for architectures that don't need page tables in their tlb range). But after that it would be interesting to see if x86 could do anything with explicit page table cache management. Thanks, Nick
On Tue, Jun 12, 2018 at 5:12 PM Nicholas Piggin <npiggin@gmail.com> wrote: > > > > And in _theory_, maybe you could have just used "invalpg" with a > > targeted address instead. In fact, I think a single invlpg invalidates > > _all_ caches for the associated MM, but don't quote me on that. Confirmed. The SDK says "INVLPG also invalidates all entries in all paging-structure caches associated with the current PCID, regardless of the linear addresses to which they correspond" so if x86 wants to do this "separate invalidation for page directory entryes", then it would want to (a) remove the __tlb_adjust_range() operation entirely from pud_free_tlb() and friends (b) instead just have a single field for "invalidate_tlb_caches", which could be a boolean, or could just be one of the addresses and then the logic would be that IFF no other tlb invalidate is done due to an actual page range, then we look at that invalidate_tlb_caches field, and do a single INVLPG instead. I still am not sure if this would actually make a difference in practice, but I guess it does mean that x86 could at least participate in some kind of scheme where we have architecture-specific actions for those page directory entries. And we could make the default behavior - if no architecture-specific tlb page directory invalidation function exists - be the current "__tlb_adjust_range()" case. So the default would be to not change behavior, and architectures could opt in to something like this. Linus
diff --git a/arch/powerpc/mm/tlb-radix.c b/arch/powerpc/mm/tlb-radix.c index 67a6e86d3e7e..06452ad701cf 100644 --- a/arch/powerpc/mm/tlb-radix.c +++ b/arch/powerpc/mm/tlb-radix.c @@ -853,8 +853,11 @@ void radix__tlb_flush(struct mmu_gather *tlb) else radix__flush_all_mm(mm); } else { - unsigned long start = tlb->start; - unsigned long end = tlb->end; + unsigned long start = tlb->page_start; + unsigned long end = tlb->page_end; + + if (end < start) + end = start; if (!tlb->need_flush_all) radix__flush_tlb_range_psize(mm, start, end, psize);
Use the page_start and page_end fields of mmu_gather to implement more precise TLB flushing. (start, end) covers the entire TLB and page table range that has been invalidated, for architectures that do not have explicit page walk cache management. page_start and page_end are just for ranges that may have TLB entries. A tlb_flush may have no pages in this range, but still requires PWC to be flushed. That is handled properly. This brings the number of tlbiel instructions required by a kernel compile from 33M to 25M, most avoided from exec->shift_arg_pages. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/mm/tlb-radix.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)