With the optimizations introduced by commit a46cc7a90fd8
("powerpc/mm/radix: Improve TLB/PWC flushes"), flush_tlb_mm() no
longer flushes the page walk cache with radix. This patch introduces
flush_all_mm(), which flushes everything, tlb and pwc, for a given mm.
Signed-off-by: Frederic Barrat <redacted>
---
Changelog:
v3: add comment to explain limitations on hash
v2: this patch is new
arch/powerpc/include/asm/book3s/64/tlbflush-hash.h | 20 ++++++++++++++++++++
arch/powerpc/include/asm/book3s/64/tlbflush-radix.h | 3 +++
arch/powerpc/include/asm/book3s/64/tlbflush.h | 15 +++++++++++++++
arch/powerpc/mm/tlb-radix.c | 6 ++++--
4 files changed, 42 insertions(+), 2 deletions(-)
The PSL and nMMU need to see all TLB invalidations for the memory
contexts used on the adapter. For the hash memory model, it is done by
making all TLBIs global as soon as the cxl driver is in use. For
radix, we need something similar, but we can refine and only convert
to global the invalidations for contexts actually used by the device.
The new mm_context_add_copro() API increments the 'active_cpus' count
for the contexts attached to the cxl adapter. As soon as there's more
than 1 active cpu, the TLBIs for the context become global. Active cpu
count must be decremented when detaching to restore locality if
possible and to avoid overflowing the counter.
The hash memory model support is somewhat limited, as we can't
decrement the active cpus count when mm_context_remove_copro() is
called, because we can't flush the TLB for a mm on hash. So TLBIs
remain global on hash.
Signed-off-by: Frederic Barrat <redacted>
Fixes: f24be42aab37 ("cxl: Add psl9 specific code")
---
Changelog:
v3: don't decrement active cpus count with hash, as we don't know how to flush
v2: Replace flush_tlb_mm() by the new flush_all_mm() to flush the TLBs
and PWCs (thanks to Ben)
arch/powerpc/include/asm/mmu_context.h | 46 ++++++++++++++++++++++++++++++++++
arch/powerpc/mm/mmu_context.c | 9 -------
drivers/misc/cxl/api.c | 22 +++++++++++++---
drivers/misc/cxl/context.c | 3 +++
drivers/misc/cxl/file.c | 19 ++++++++++++--
5 files changed, 85 insertions(+), 14 deletions(-)
@@ -331,9 +332,12 @@ int cxl_start_context(struct cxl_context *ctx, u64 wed,/* ensure this mm_struct can't be freed */cxl_context_mm_count_get(ctx);-/* decrement the use count */-if(ctx->mm)+if(ctx->mm){+/* decrement the use count from above */mmput(ctx->mm);+/* make TLBIs for this context global */+mm_context_add_copro(ctx->mm);+}}/*
@@ -342,13 +346,25 @@ int cxl_start_context(struct cxl_context *ctx, u64 wed,*/cxl_ctx_get();+/*+*BarrierisneededtomakesureallTLBIsareglobalbefore+*weattachandthecontextstartsbeingusedbytheadapter.+*+*Neededaftermm_context_add_copro()forradixand+*cxl_ctx_get()forhash/p8+*/+smp_mb();+if((rc=cxl_ops->attach_process(ctx,kernel,wed,0))){put_pid(ctx->pid);ctx->pid=NULL;cxl_adapter_context_put(ctx->afu->adapter);cxl_ctx_put();-if(task)+if(task){cxl_context_mm_count_put(ctx);+if(ctx->mm)+mm_context_remove_copro(ctx->mm);+}gotoout;}
@@ -267,6 +268,8 @@ int __detach_context(struct cxl_context *ctx)/* Decrease the mm count on the context */cxl_context_mm_count_put(ctx);+if(ctx->mm)+mm_context_remove_copro(ctx->mm);ctx->mm=NULL;return0;
@@ -220,9 +221,12 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,/* ensure this mm_struct can't be freed */cxl_context_mm_count_get(ctx);-/* decrement the use count */-if(ctx->mm)+if(ctx->mm){+/* decrement the use count from above */mmput(ctx->mm);+/* make TLBIs for this context global */+mm_context_add_copro(ctx->mm);+}/**Incrementdriverusecount.EnablesglobalTLBIsforhash
@@ -230,6 +234,15 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,*/cxl_ctx_get();+/*+*BarrierisneededtomakesureallTLBIsareglobalbefore+*weattachandthecontextstartsbeingusedbytheadapter.+*+*Neededaftermm_context_add_copro()forradixand+*cxl_ctx_get()forhash/p8+*/+smp_mb();+trace_cxl_attach(ctx,work.work_element_descriptor,work.num_interrupts,amr);if((rc=cxl_ops->attach_process(ctx,false,work.work_element_descriptor,
@@ -240,6 +253,8 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,ctx->pid=NULL;cxl_ctx_put();cxl_context_mm_count_put(ctx);+if(ctx->mm)+mm_context_remove_copro(ctx->mm);gotoout;}
The PSL and nMMU need to see all TLB invalidations for the memory
contexts used on the adapter. For the hash memory model, it is done by
making all TLBIs global as soon as the cxl driver is in use. For
radix, we need something similar, but we can refine and only convert
to global the invalidations for contexts actually used by the device.
The new mm_context_add_copro() API increments the 'active_cpus' count
for the contexts attached to the cxl adapter. As soon as there's more
than 1 active cpu, the TLBIs for the context become global. Active cpu
count must be decremented when detaching to restore locality if
possible and to avoid overflowing the counter.
The hash memory model support is somewhat limited, as we can't
decrement the active cpus count when mm_context_remove_copro() is
called, because we can't flush the TLB for a mm on hash. So TLBIs
remain global on hash.
Sorry I didn't look at this earlier and just wading in here a bit, but
what do you think of using mmu notifiers for invalidating nMMU and
coprocessor caches, rather than put the details into the host MMU
management? npu-dma.c already looks to have almost everything covered
with its notifiers (in that it wouldn't have to rely on tlbie coming
from host MMU code).
This change is not too bad today, but if we get to more complicated
MMU/nMMU TLB management like directed invalidation of particular units,
then putting more knowledge into the host code will end up being
complex I think.
I also want to also do optimizations on the core code that assumes we
only have to take care of other CPUs, e.g.,
https://patchwork.ozlabs.org/patch/811068/
Or, another example, directed IPI invalidations from the mm_cpumask
bitmap.
I realize you want to get something merged! For the merge window and
backports this seems fine. I think it would be nice soon afterwards to
get nMMU knowledge out of the core code... Though I also realize with
our tlbie instruction that does everything then it may be tricky to
make a really optimal notifier.
Thanks,
Nick
Signed-off-by: Frederic Barrat <redacted>
Fixes: f24be42aab37 ("cxl: Add psl9 specific code")
---
Changelog:
v3: don't decrement active cpus count with hash, as we don't know how to flush
v2: Replace flush_tlb_mm() by the new flush_all_mm() to flush the TLBs
and PWCs (thanks to Ben)
The PSL and nMMU need to see all TLB invalidations for the memory
contexts used on the adapter. For the hash memory model, it is done by
making all TLBIs global as soon as the cxl driver is in use. For
radix, we need something similar, but we can refine and only convert
to global the invalidations for contexts actually used by the device.
The new mm_context_add_copro() API increments the 'active_cpus' count
for the contexts attached to the cxl adapter. As soon as there's more
than 1 active cpu, the TLBIs for the context become global. Active cpu
count must be decremented when detaching to restore locality if
possible and to avoid overflowing the counter.
The hash memory model support is somewhat limited, as we can't
decrement the active cpus count when mm_context_remove_copro() is
called, because we can't flush the TLB for a mm on hash. So TLBIs
remain global on hash.
Sorry I didn't look at this earlier and just wading in here a bit, but
what do you think of using mmu notifiers for invalidating nMMU and
coprocessor caches, rather than put the details into the host MMU
management? npu-dma.c already looks to have almost everything covered
with its notifiers (in that it wouldn't have to rely on tlbie coming
from host MMU code).
Does npu-dma.c really do mmio nMMU invalidations? My understanding was
that those atsd_launch operations are really targeted at the device
behind the NPU, i.e. the nvidia card.
At some point, it was not possible to do mmio invalidations on the nMMU.
At least on dd1. I'm checking with the nMMU team the status on dd2.
Alistair: is your code really doing a nMMU invalidation? Considering
you're trying to also reuse the mm_context_add_copro() from this patch,
I think I know the answer.
There are also other components relying on broadcasted invalidations
from hardware: the PSL (for capi FPGA) and the XSL on the Mellanox CX5
card, when in capi mode. They rely on hardware TLBIs, snooped and
forwarded to them by the CAPP.
For the PSL, we do have a mmio interface to do targeted invalidations,
but it was removed from the capi architecture (and left as a debug
feature for our PSL implementation), because the nMMU would be out of
sync with the PSL (due to the lack of interface to sync the nMMU, as
mentioned above).
For the XSL on the Mellanox CX5, it's even more complicated. AFAIK, they
do have a way to trigger invalidations through software, though the
interface is private and Mellanox would have to be involved. They've
also stated the performance is much worse through software invalidation.
Another consideration is performance. Which is best? Short of having
real numbers, it's probably hard to know for sure.
So the road of getting rid of hardware invalidations for external
components, if at all possible or even desirable, may be long.
Fred
Signed-off-by: Frederic Barrat <redacted>
Fixes: f24be42aab37 ("cxl: Add psl9 specific code")
---
Changelog:
v3: don't decrement active cpus count with hash, as we don't know how to flush
v2: Replace flush_tlb_mm() by the new flush_all_mm() to flush the TLBs
and PWCs (thanks to Ben)
The PSL and nMMU need to see all TLB invalidations for the memory
contexts used on the adapter. For the hash memory model, it is done by
making all TLBIs global as soon as the cxl driver is in use. For
radix, we need something similar, but we can refine and only convert
to global the invalidations for contexts actually used by the device.
The new mm_context_add_copro() API increments the 'active_cpus' count
for the contexts attached to the cxl adapter. As soon as there's more
than 1 active cpu, the TLBIs for the context become global. Active cpu
count must be decremented when detaching to restore locality if
possible and to avoid overflowing the counter.
The hash memory model support is somewhat limited, as we can't
decrement the active cpus count when mm_context_remove_copro() is
called, because we can't flush the TLB for a mm on hash. So TLBIs
remain global on hash.
Sorry I didn't look at this earlier and just wading in here a bit, but
what do you think of using mmu notifiers for invalidating nMMU and
coprocessor caches, rather than put the details into the host MMU
management? npu-dma.c already looks to have almost everything covered
with its notifiers (in that it wouldn't have to rely on tlbie coming
from host MMU code).
Does npu-dma.c really do mmio nMMU invalidations?
No, but it does do a flush_tlb_mm there to do a tlbie (probably
buggy in some cases and does tlbiel without this patch of yours).
But the point is when you control the flushing you don't have to
mess with making the core flush code give you tlbies.
Just add a flush_nmmu_mm or something that does what you need.
If you can make a more targeted nMMU invalidate, then that's
even better.
One downside at first I thought is that the core code might already
do a broadcast tlbie, then the mmu notifier does not easily know
about that so it will do a second one which will be suboptimal.
Possibly we could add some flag or state so the nmmu flush can
avoid the second one.
But now that I look again, the NPU code has this comment:
/*
* Unfortunately the nest mmu does not support flushing specific
* addresses so we have to flush the whole mm.
*/
Which seems to indicate that you can't rely on core code to give
you full flushes because for range flushing it is possible that the
core code will do it with address flushes. Or am I missing something?
So it seems you really do need to always issue a full PID tlbie from
a notifier.
My understanding was
that those atsd_launch operations are really targeted at the device
behind the NPU, i.e. the nvidia card.
At some point, it was not possible to do mmio invalidations on the nMMU.
At least on dd1. I'm checking with the nMMU team the status on dd2.
Alistair: is your code really doing a nMMU invalidation? Considering
you're trying to also reuse the mm_context_add_copro() from this patch,
I think I know the answer.
There are also other components relying on broadcasted invalidations
from hardware: the PSL (for capi FPGA) and the XSL on the Mellanox CX5
card, when in capi mode. They rely on hardware TLBIs, snooped and
forwarded to them by the CAPP.
For the PSL, we do have a mmio interface to do targeted invalidations,
but it was removed from the capi architecture (and left as a debug
feature for our PSL implementation), because the nMMU would be out of
sync with the PSL (due to the lack of interface to sync the nMMU, as
mentioned above).
For the XSL on the Mellanox CX5, it's even more complicated. AFAIK, they
do have a way to trigger invalidations through software, though the
interface is private and Mellanox would have to be involved. They've
also stated the performance is much worse through software invalidation.
Okay, point is I think the nMMU and agent drivers will be in a better
position to handle all that. I don't see that flushing from your notifier
means that you can't issue a tlbie to do it.
Another consideration is performance. Which is best? Short of having
real numbers, it's probably hard to know for sure.
Let's come to that if we agree on a way to go. I *think* we can make it
at least no worse than we have today, using tlbie and possibly some small
changes to generic code callers.
Thanks,
Nick
The PSL and nMMU need to see all TLB invalidations for the memory
contexts used on the adapter. For the hash memory model, it is done by
making all TLBIs global as soon as the cxl driver is in use. For
radix, we need something similar, but we can refine and only convert
to global the invalidations for contexts actually used by the device.
The new mm_context_add_copro() API increments the 'active_cpus' count
for the contexts attached to the cxl adapter. As soon as there's more
than 1 active cpu, the TLBIs for the context become global. Active cpu
count must be decremented when detaching to restore locality if
possible and to avoid overflowing the counter.
The hash memory model support is somewhat limited, as we can't
decrement the active cpus count when mm_context_remove_copro() is
called, because we can't flush the TLB for a mm on hash. So TLBIs
remain global on hash.
Sorry I didn't look at this earlier and just wading in here a bit, but
what do you think of using mmu notifiers for invalidating nMMU and
coprocessor caches, rather than put the details into the host MMU
management? npu-dma.c already looks to have almost everything covered
with its notifiers (in that it wouldn't have to rely on tlbie coming
from host MMU code).
Does npu-dma.c really do mmio nMMU invalidations?
No, but it does do a flush_tlb_mm there to do a tlbie (probably
buggy in some cases and does tlbiel without this patch of yours).
But the point is when you control the flushing you don't have to
mess with making the core flush code give you tlbies.
Just add a flush_nmmu_mm or something that does what you need.
If you can make a more targeted nMMU invalidate, then that's
even better.
One downside at first I thought is that the core code might already
do a broadcast tlbie, then the mmu notifier does not easily know
about that so it will do a second one which will be suboptimal.
Possibly we could add some flag or state so the nmmu flush can
avoid the second one.
But now that I look again, the NPU code has this comment:
/*
* Unfortunately the nest mmu does not support flushing specific
* addresses so we have to flush the whole mm.
*/
Which seems to indicate that you can't rely on core code to give
you full flushes because for range flushing it is possible that the
core code will do it with address flushes. Or am I missing something?
So it seems you really do need to always issue a full PID tlbie from
a notifier.
Oh I see, actually it's fixed in newer firmware and there's a patch
out for it.
Okay, so the nMMU can take address tlbie, in that case it's not a
correctness issue (except for old firmware that still has the bug).
The PSL and nMMU need to see all TLB invalidations for the memory
contexts used on the adapter. For the hash memory model, it is done by
making all TLBIs global as soon as the cxl driver is in use. For
radix, we need something similar, but we can refine and only convert
to global the invalidations for contexts actually used by the device.
The new mm_context_add_copro() API increments the 'active_cpus' count
for the contexts attached to the cxl adapter. As soon as there's more
than 1 active cpu, the TLBIs for the context become global. Active cpu
count must be decremented when detaching to restore locality if
possible and to avoid overflowing the counter.
The hash memory model support is somewhat limited, as we can't
decrement the active cpus count when mm_context_remove_copro() is
called, because we can't flush the TLB for a mm on hash. So TLBIs
remain global on hash.
Sorry I didn't look at this earlier and just wading in here a bit, but
what do you think of using mmu notifiers for invalidating nMMU and
coprocessor caches, rather than put the details into the host MMU
management? npu-dma.c already looks to have almost everything covered
with its notifiers (in that it wouldn't have to rely on tlbie coming
from host MMU code).
Sorry, just finding time to catch up on this. From subsequent emails it looks
like you may have figured this out. The TLB flush in npu-dma.c is a workaround
for a HW issue rather than there to explicitly manage the NMMU caches. The
intent for NPU was always to have the NMMU snoop normal core tlbies rather than
do it via notifiers. A subsequent patch series
(https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=1681) removes
this flush now that the HW issue has been worked around via a FW fix.
I agree this is something we could look into optimising in the medium term, but
for the moment it would be good if we could get this series merged.
- Alistair
I have tested the non-cxl specific parts
(mm_context_add_copro/mm_context_remove_copro) with this series -
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=1681 - and it
works well for npu.
Tested-by: Alistair Popple <redacted>
On Sun, 3 Sep 2017 08:15:13 PM Frederic Barrat wrote:
quoted hunk
The PSL and nMMU need to see all TLB invalidations for the memory
contexts used on the adapter. For the hash memory model, it is done by
making all TLBIs global as soon as the cxl driver is in use. For
radix, we need something similar, but we can refine and only convert
to global the invalidations for contexts actually used by the device.
The new mm_context_add_copro() API increments the 'active_cpus' count
for the contexts attached to the cxl adapter. As soon as there's more
than 1 active cpu, the TLBIs for the context become global. Active cpu
count must be decremented when detaching to restore locality if
possible and to avoid overflowing the counter.
The hash memory model support is somewhat limited, as we can't
decrement the active cpus count when mm_context_remove_copro() is
called, because we can't flush the TLB for a mm on hash. So TLBIs
remain global on hash.
Signed-off-by: Frederic Barrat <redacted>
Fixes: f24be42aab37 ("cxl: Add psl9 specific code")
---
Changelog:
v3: don't decrement active cpus count with hash, as we don't know how to flush
v2: Replace flush_tlb_mm() by the new flush_all_mm() to flush the TLBs
and PWCs (thanks to Ben)
arch/powerpc/include/asm/mmu_context.h | 46 ++++++++++++++++++++++++++++++++++
arch/powerpc/mm/mmu_context.c | 9 -------
drivers/misc/cxl/api.c | 22 +++++++++++++---
drivers/misc/cxl/context.c | 3 +++
drivers/misc/cxl/file.c | 19 ++++++++++++--
5 files changed, 85 insertions(+), 14 deletions(-)
@@ -331,9 +332,12 @@ int cxl_start_context(struct cxl_context *ctx, u64 wed,/* ensure this mm_struct can't be freed */cxl_context_mm_count_get(ctx);-/* decrement the use count */-if(ctx->mm)+if(ctx->mm){+/* decrement the use count from above */mmput(ctx->mm);+/* make TLBIs for this context global */+mm_context_add_copro(ctx->mm);+}}/*
@@ -342,13 +346,25 @@ int cxl_start_context(struct cxl_context *ctx, u64 wed,*/cxl_ctx_get();+/*+*BarrierisneededtomakesureallTLBIsareglobalbefore+*weattachandthecontextstartsbeingusedbytheadapter.+*+*Neededaftermm_context_add_copro()forradixand+*cxl_ctx_get()forhash/p8+*/+smp_mb();+if((rc=cxl_ops->attach_process(ctx,kernel,wed,0))){put_pid(ctx->pid);ctx->pid=NULL;cxl_adapter_context_put(ctx->afu->adapter);cxl_ctx_put();-if(task)+if(task){cxl_context_mm_count_put(ctx);+if(ctx->mm)+mm_context_remove_copro(ctx->mm);+}gotoout;}
@@ -267,6 +268,8 @@ int __detach_context(struct cxl_context *ctx)/* Decrease the mm count on the context */cxl_context_mm_count_put(ctx);+if(ctx->mm)+mm_context_remove_copro(ctx->mm);ctx->mm=NULL;return0;
@@ -220,9 +221,12 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,/* ensure this mm_struct can't be freed */cxl_context_mm_count_get(ctx);-/* decrement the use count */-if(ctx->mm)+if(ctx->mm){+/* decrement the use count from above */mmput(ctx->mm);+/* make TLBIs for this context global */+mm_context_add_copro(ctx->mm);+}/**Incrementdriverusecount.EnablesglobalTLBIsforhash
@@ -230,6 +234,15 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,*/cxl_ctx_get();+/*+*BarrierisneededtomakesureallTLBIsareglobalbefore+*weattachandthecontextstartsbeingusedbytheadapter.+*+*Neededaftermm_context_add_copro()forradixand+*cxl_ctx_get()forhash/p8+*/+smp_mb();+trace_cxl_attach(ctx,work.work_element_descriptor,work.num_interrupts,amr);if((rc=cxl_ops->attach_process(ctx,false,work.work_element_descriptor,
@@ -240,6 +253,8 @@ static long afu_ioctl_start_work(struct cxl_context *ctx,ctx->pid=NULL;cxl_ctx_put();cxl_context_mm_count_put(ctx);+if(ctx->mm)+mm_context_remove_copro(ctx->mm);gotoout;}
+static inline void hash__local_flush_all_mm(struct mm_struct *mm)
+{
+ /*
+ * There's no Page Walk Cache for hash, so what is needed is
+ * the same as flush_tlb_mm(), which doesn't really make sense
+ * with hash. So the only thing we could do is flush the
+ * entire LPID! Punt for now, as it's not being used.
+ */
Do you think it is worth putting a WARN_ON_ONCE here if we're asserting this
isn't used on hash?
Otherwise looks good and is also needed for NPU.
Reviewed-By: Alistair Popple <redacted>
quoted hunk
+}
+
+static inline void hash__flush_all_mm(struct mm_struct *mm)
+{
+ /*
+ * There's no Page Walk Cache for hash, so what is needed is
+ * the same as flush_tlb_mm(), which doesn't really make sense
+ * with hash. So the only thing we could do is flush the
+ * entire LPID! Punt for now, as it's not being used.
+ */
+}
+
static inline void hash__local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long vmaddr)
{
+static inline void hash__local_flush_all_mm(struct mm_struct *mm)
+{
+ /*
+ * There's no Page Walk Cache for hash, so what is needed is
+ * the same as flush_tlb_mm(), which doesn't really make sense
+ * with hash. So the only thing we could do is flush the
+ * entire LPID! Punt for now, as it's not being used.
+ */
Do you think it is worth putting a WARN_ON_ONCE here if we're asserting this
isn't used on hash?
I toyed with the idea. The reason I didn't add it was because
hash__local_flush_tlb_mm() and hash__flush_tlb_mm() don't have one, yet
it's also not supported. And I had faith in a developer thinking about
using it would see the comment.
I was actually pretty close to have hash__local_flush_all_mm() call
directly hash__local_flush_tlb_mm(), since the "all" stands for "pwc and
tlb" and pwc doesn't exist for hash. But that doesn't do us any good for
the time being.
Michael: any preference?
Side note: I'm under the impression that flush_tlb_mm() may be called on
hash, even though it does nothing. kernel/fork.c, dup_mmap() and
potentially another (through cscope).
Fred
Otherwise looks good and is also needed for NPU.
Reviewed-By: Alistair Popple <redacted>
quoted
+}
+
+static inline void hash__flush_all_mm(struct mm_struct *mm)
+{
+ /*
+ * There's no Page Walk Cache for hash, so what is needed is
+ * the same as flush_tlb_mm(), which doesn't really make sense
+ * with hash. So the only thing we could do is flush the
+ * entire LPID! Punt for now, as it's not being used.
+ */
+}
+
static inline void hash__local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long vmaddr)
{
From: Michael Ellerman <hidden> Date: 2017-10-05 04:21:40
On Sun, 2017-09-03 at 18:15:12 UTC, Frederic Barrat wrote:
With the optimizations introduced by commit a46cc7a90fd8
("powerpc/mm/radix: Improve TLB/PWC flushes"), flush_tlb_mm() no
longer flushes the page walk cache with radix. This patch introduces
flush_all_mm(), which flushes everything, tlb and pwc, for a given mm.
Signed-off-by: Frederic Barrat <redacted>
Reviewed-By: Alistair Popple <redacted>