* [RFC PATCH 1/3] um/x86: remove ldt mutex and use mmap lock instead
2023-09-22 22:37 [RFC PATCH 0/3] um: clean up mm creation - another attempt Johannes Berg
@ 2023-09-22 22:37 ` Johannes Berg
2023-09-22 22:37 ` [RFC PATCH 2/3] um: clean up init_new_context() Johannes Berg
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Johannes Berg @ 2023-09-22 22:37 UTC (permalink / raw)
To: linux-um; +Cc: Anton Ivanov, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
There's really not much value in having this as a
separate lock, and having it makes it harder to
clean up the init_new_context() semantics to not
rely on 'current', even if dup_mm() is really only
called with 'current->mm' as the source today.
Replace the locking accordingly with mmap read and
write lock, and remove it from init_new_ldt()
entirely since it already holds the source lock.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
arch/x86/um/asm/mm_context.h | 1 -
arch/x86/um/ldt.c | 14 ++++----------
2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/arch/x86/um/asm/mm_context.h b/arch/x86/um/asm/mm_context.h
index dc32dc023c2f..2b1a76a87e14 100644
--- a/arch/x86/um/asm/mm_context.h
+++ b/arch/x86/um/asm/mm_context.h
@@ -25,7 +25,6 @@ struct ldt_entry {
typedef struct uml_ldt {
int entry_count;
- struct mutex lock;
union {
struct ldt_entry * pages[LDT_PAGES_MAX];
struct ldt_entry entries[LDT_DIRECT_ENTRIES];
diff --git a/arch/x86/um/ldt.c b/arch/x86/um/ldt.c
index 255a44dd415a..6b8e368f779c 100644
--- a/arch/x86/um/ldt.c
+++ b/arch/x86/um/ldt.c
@@ -65,7 +65,7 @@ static int read_ldt(void __user * ptr, unsigned long bytecount)
bytecount = LDT_ENTRY_SIZE*LDT_ENTRIES;
err = bytecount;
- mutex_lock(&ldt->lock);
+ mmap_read_lock(current->mm);
if (ldt->entry_count <= LDT_DIRECT_ENTRIES) {
size = LDT_ENTRY_SIZE*LDT_DIRECT_ENTRIES;
if (size > bytecount)
@@ -89,7 +89,7 @@ static int read_ldt(void __user * ptr, unsigned long bytecount)
ptr += size;
}
}
- mutex_unlock(&ldt->lock);
+ mmap_read_unlock(current->mm);
if (bytecount == 0 || err == -EFAULT)
goto out;
@@ -146,7 +146,7 @@ static int write_ldt(void __user * ptr, unsigned long bytecount, int func)
goto out;
}
- mutex_lock(&ldt->lock);
+ mmap_write_lock(current->mm);
err = write_ldt_entry(mm_idp, func, &ldt_info, &addr, 1);
if (err)
@@ -201,7 +201,7 @@ static int write_ldt(void __user * ptr, unsigned long bytecount, int func)
err = 0;
out_unlock:
- mutex_unlock(&ldt->lock);
+ mmap_write_unlock(current->mm);
out:
return err;
}
@@ -305,9 +305,6 @@ long init_new_ldt(struct mm_context *new_mm, struct mm_context *from_mm)
long page, err=0;
void *addr = NULL;
-
- mutex_init(&new_mm->arch.ldt.lock);
-
if (!from_mm) {
memset(&desc, 0, sizeof(desc));
/*
@@ -334,7 +331,6 @@ long init_new_ldt(struct mm_context *new_mm, struct mm_context *from_mm)
* i.e., we have to use the stub for modify_ldt, which
* can't handle the big read buffer of up to 64kB.
*/
- mutex_lock(&from_mm->arch.ldt.lock);
if (from_mm->arch.ldt.entry_count <= LDT_DIRECT_ENTRIES)
memcpy(new_mm->arch.ldt.u.entries, from_mm->arch.ldt.u.entries,
sizeof(new_mm->arch.ldt.u.entries));
@@ -353,8 +349,6 @@ long init_new_ldt(struct mm_context *new_mm, struct mm_context *from_mm)
}
}
new_mm->arch.ldt.entry_count = from_mm->arch.ldt.entry_count;
- mutex_unlock(&from_mm->arch.ldt.lock);
-
out:
return err;
}
--
2.41.0
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC PATCH 2/3] um: clean up init_new_context()
2023-09-22 22:37 [RFC PATCH 0/3] um: clean up mm creation - another attempt Johannes Berg
2023-09-22 22:37 ` [RFC PATCH 1/3] um/x86: remove ldt mutex and use mmap lock instead Johannes Berg
@ 2023-09-22 22:37 ` Johannes Berg
2023-09-22 22:37 ` [RFC PATCH 3/3] um: don't force-flush in mm/userspace process start Johannes Berg
2023-09-25 13:29 ` [RFC PATCH 0/3] um: clean up mm creation - another attempt Anton Ivanov
3 siblings, 0 replies; 19+ messages in thread
From: Johannes Berg @ 2023-09-22 22:37 UTC (permalink / raw)
To: linux-um; +Cc: Anton Ivanov, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
This function gets a context that's already copied from
the old context if making a copy, or memset to 0 if not.
This means we don't need to play games with current,
even if those games are OK now since dup_mm() is only
ever called with current->mm as the source.
Also memset() our context to 0 since we don't have in
there anything that we need to keep - except the LDT but
we have to copy that separately, so make a temporary
local copy.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
arch/um/kernel/skas/mmu.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c
index 656fe16c9b63..ea0c37b92581 100644
--- a/arch/um/kernel/skas/mmu.c
+++ b/arch/um/kernel/skas/mmu.c
@@ -16,6 +16,7 @@
int init_new_context(struct task_struct *task, struct mm_struct *mm)
{
+ struct mm_context _from_mm;
struct mm_context *from_mm = NULL;
struct mm_context *to_mm = &mm->context;
unsigned long stack = 0;
@@ -25,15 +26,26 @@ int init_new_context(struct task_struct *task, struct mm_struct *mm)
if (stack == 0)
goto out;
+ /*
+ * If the kernel wants a copy, it already copied the entire context.
+ * If not, it's all memset to 0.
+ * So we can detect here whether or not we should copy, and have the
+ * pid we should copy _from_ in our own context struct of the new mm.
+ */
+ if (to_mm->id.u.pid) {
+ _from_mm = *to_mm;
+ from_mm = &_from_mm;
+ }
+
+ memset(to_mm, 0, sizeof(*to_mm));
to_mm->id.stack = stack;
- if (current->mm != NULL && current->mm != &init_mm)
- from_mm = ¤t->mm->context;
block_signals_trace();
if (from_mm)
to_mm->id.u.pid = copy_context_skas0(stack,
from_mm->id.u.pid);
- else to_mm->id.u.pid = start_userspace(stack);
+ else
+ to_mm->id.u.pid = start_userspace(stack);
unblock_signals_trace();
if (to_mm->id.u.pid < 0) {
--
2.41.0
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC PATCH 3/3] um: don't force-flush in mm/userspace process start
2023-09-22 22:37 [RFC PATCH 0/3] um: clean up mm creation - another attempt Johannes Berg
2023-09-22 22:37 ` [RFC PATCH 1/3] um/x86: remove ldt mutex and use mmap lock instead Johannes Berg
2023-09-22 22:37 ` [RFC PATCH 2/3] um: clean up init_new_context() Johannes Berg
@ 2023-09-22 22:37 ` Johannes Berg
2023-09-25 13:29 ` [RFC PATCH 0/3] um: clean up mm creation - another attempt Anton Ivanov
3 siblings, 0 replies; 19+ messages in thread
From: Johannes Berg @ 2023-09-22 22:37 UTC (permalink / raw)
To: linux-um; +Cc: Anton Ivanov, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
The reason to flush here is to resynchronize the mm after
a copy. For exec we don't need to since flush_thread()
will anyway clear everything.
Thus, we can just do this in arch_dup_mmap(), since when
that's invoked, the new mm has been setup sufficiently.
This isn't true if we just flush in init_new_context().
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
arch/um/include/asm/Kbuild | 1 -
arch/um/include/asm/mm_hooks.h | 22 ++++++++++++++++++++++
arch/um/include/asm/mmu_context.h | 2 +-
arch/um/kernel/process.c | 2 --
arch/um/kernel/skas/mmu.c | 7 +++++++
arch/um/kernel/tlb.c | 5 +----
6 files changed, 31 insertions(+), 8 deletions(-)
create mode 100644 arch/um/include/asm/mm_hooks.h
diff --git a/arch/um/include/asm/Kbuild b/arch/um/include/asm/Kbuild
index b2d834a29f3a..de8d82a6fd7b 100644
--- a/arch/um/include/asm/Kbuild
+++ b/arch/um/include/asm/Kbuild
@@ -26,5 +26,4 @@ generic-y += switch_to.h
generic-y += topology.h
generic-y += trace_clock.h
generic-y += kprobes.h
-generic-y += mm_hooks.h
generic-y += vga.h
diff --git a/arch/um/include/asm/mm_hooks.h b/arch/um/include/asm/mm_hooks.h
new file mode 100644
index 000000000000..b1016520c5b8
--- /dev/null
+++ b/arch/um/include/asm/mm_hooks.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_UM_MM_HOOKS_H
+#define _ASM_UM_MM_HOOKS_H
+
+int arch_dup_mmap(struct mm_struct *oldmm, struct mm_struct *mm);
+
+static inline void arch_exit_mmap(struct mm_struct *mm)
+{
+}
+
+static inline void arch_unmap(struct mm_struct *mm,
+ unsigned long start, unsigned long end)
+{
+}
+
+static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
+ bool write, bool execute, bool foreign)
+{
+ /* by default, allow everything */
+ return true;
+}
+#endif /* _ASM_UM_MM_HOOKS_H */
diff --git a/arch/um/include/asm/mmu_context.h b/arch/um/include/asm/mmu_context.h
index 68e2eb9cfb47..8668861d4a85 100644
--- a/arch/um/include/asm/mmu_context.h
+++ b/arch/um/include/asm/mmu_context.h
@@ -13,7 +13,7 @@
#include <asm/mm_hooks.h>
#include <asm/mmu.h>
-extern void force_flush_all(void);
+void force_flush_all(struct mm_struct *mm);
#define activate_mm activate_mm
static inline void activate_mm(struct mm_struct *old, struct mm_struct *new)
diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
index 6daffb9d8a8d..d7545ba6522a 100644
--- a/arch/um/kernel/process.c
+++ b/arch/um/kernel/process.c
@@ -139,8 +139,6 @@ void new_thread_handler(void)
/* Called magically, see new_thread_handler above */
void fork_handler(void)
{
- force_flush_all();
-
schedule_tail(current->thread.prev_sched);
/*
diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c
index ea0c37b92581..97e731180f70 100644
--- a/arch/um/kernel/skas/mmu.c
+++ b/arch/um/kernel/skas/mmu.c
@@ -10,6 +10,7 @@
#include <asm/pgalloc.h>
#include <asm/sections.h>
+#include <asm/mmu_context.h>
#include <as-layout.h>
#include <os.h>
#include <skas.h>
@@ -69,6 +70,12 @@ int init_new_context(struct task_struct *task, struct mm_struct *mm)
return ret;
}
+int arch_dup_mmap(struct mm_struct *oldmm, struct mm_struct *mm)
+{
+ force_flush_all(mm);
+ return 0;
+}
+
void destroy_context(struct mm_struct *mm)
{
struct mm_context *mmu = &mm->context;
diff --git a/arch/um/kernel/tlb.c b/arch/um/kernel/tlb.c
index 34ec8e677fb9..7c0161321fd9 100644
--- a/arch/um/kernel/tlb.c
+++ b/arch/um/kernel/tlb.c
@@ -600,14 +600,11 @@ void flush_tlb_mm(struct mm_struct *mm)
fix_range(mm, vma->vm_start, vma->vm_end, 0);
}
-void force_flush_all(void)
+void force_flush_all(struct mm_struct *mm)
{
- struct mm_struct *mm = current->mm;
struct vm_area_struct *vma;
VMA_ITERATOR(vmi, mm, 0);
- mmap_read_lock(mm);
for_each_vma(vmi, vma)
fix_range(mm, vma->vm_start, vma->vm_end, 1);
- mmap_read_unlock(mm);
}
--
2.41.0
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-22 22:37 [RFC PATCH 0/3] um: clean up mm creation - another attempt Johannes Berg
` (2 preceding siblings ...)
2023-09-22 22:37 ` [RFC PATCH 3/3] um: don't force-flush in mm/userspace process start Johannes Berg
@ 2023-09-25 13:29 ` Anton Ivanov
2023-09-25 13:33 ` Johannes Berg
3 siblings, 1 reply; 19+ messages in thread
From: Anton Ivanov @ 2023-09-25 13:29 UTC (permalink / raw)
To: Johannes Berg, linux-um
I have rebased the preempt patch on top of these series.
PREEMPT works with some performance decrease.
VOLUNTARY deadlocks early in boot around the time it starts loading modules.
non-preemptible deadlocks very early in boot.
I am going to investigate and come back with a revised PREEMPT patch (hopefully, this will also flush out any remaining gremlins).
A.
On 22/09/2023 23:37, Johannes Berg wrote:
> This shouldn't pessimize since we keep copy_context_skas0() and
> the setup there, but still clean up the code there to not rely on
> current, and move the force_flush_all() to a more appropriate
> place, which fixes the issue with that in the fork_handler().
>
> johannes
>
>
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-25 13:29 ` [RFC PATCH 0/3] um: clean up mm creation - another attempt Anton Ivanov
@ 2023-09-25 13:33 ` Johannes Berg
2023-09-25 13:34 ` Anton Ivanov
2023-09-25 14:27 ` Anton Ivanov
0 siblings, 2 replies; 19+ messages in thread
From: Johannes Berg @ 2023-09-25 13:33 UTC (permalink / raw)
To: Anton Ivanov, linux-um
On Mon, 2023-09-25 at 14:29 +0100, Anton Ivanov wrote:
> I have rebased the preempt patch on top of these series.
>
> PREEMPT works with some performance decrease.
>
> VOLUNTARY deadlocks early in boot around the time it starts loading modules.
>
> non-preemptible deadlocks very early in boot.
>
Well I guess that means there's still some issue in here? Hmm.
Now I don't understand anything anymore, I guess.
johannes
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-25 13:33 ` Johannes Berg
@ 2023-09-25 13:34 ` Anton Ivanov
2023-09-25 14:27 ` Anton Ivanov
1 sibling, 0 replies; 19+ messages in thread
From: Anton Ivanov @ 2023-09-25 13:34 UTC (permalink / raw)
To: Johannes Berg, linux-um
On 25/09/2023 14:33, Johannes Berg wrote:
> On Mon, 2023-09-25 at 14:29 +0100, Anton Ivanov wrote:
>> I have rebased the preempt patch on top of these series.
>>
>> PREEMPT works with some performance decrease.
>>
>> VOLUNTARY deadlocks early in boot around the time it starts loading modules.
>>
>> non-preemptible deadlocks very early in boot.
>>
>
> Well I guess that means there's still some issue in here? Hmm.
>
> Now I don't understand anything anymore, I guess.
UML VM and process handling.
And here be dragons :)
>
> johannes
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-25 13:33 ` Johannes Berg
2023-09-25 13:34 ` Anton Ivanov
@ 2023-09-25 14:27 ` Anton Ivanov
2023-09-25 14:44 ` Johannes Berg
1 sibling, 1 reply; 19+ messages in thread
From: Anton Ivanov @ 2023-09-25 14:27 UTC (permalink / raw)
To: Johannes Berg, linux-um
On 25/09/2023 14:33, Johannes Berg wrote:
> On Mon, 2023-09-25 at 14:29 +0100, Anton Ivanov wrote:
>> I have rebased the preempt patch on top of these series.
>>
>> PREEMPT works with some performance decrease.
>>
>> VOLUNTARY deadlocks early in boot around the time it starts loading modules.
>>
>> non-preemptible deadlocks very early in boot.
>>
>
> Well I guess that means there's still some issue in here? Hmm.
>
> Now I don't understand anything anymore, I guess.
PEBKAC. The tree got corrupted somewhere during rebase. Reapplying everything on top of a clean master fixed it.
So it all works.
With some performance penalties compared to the old approach, but works.
>
> johannes
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-25 14:27 ` Anton Ivanov
@ 2023-09-25 14:44 ` Johannes Berg
2023-09-25 15:20 ` Anton Ivanov
0 siblings, 1 reply; 19+ messages in thread
From: Johannes Berg @ 2023-09-25 14:44 UTC (permalink / raw)
To: Anton Ivanov, linux-um
On Mon, 2023-09-25 at 15:27 +0100, Anton Ivanov wrote:
> On 25/09/2023 14:33, Johannes Berg wrote:
> > On Mon, 2023-09-25 at 14:29 +0100, Anton Ivanov wrote:
> > > I have rebased the preempt patch on top of these series.
> > >
> > > PREEMPT works with some performance decrease.
> > >
> > > VOLUNTARY deadlocks early in boot around the time it starts loading modules.
> > >
> > > non-preemptible deadlocks very early in boot.
> > >
> >
> > Well I guess that means there's still some issue in here? Hmm.
> >
> > Now I don't understand anything anymore, I guess.
>
> PEBKAC. The tree got corrupted somewhere during rebase. Reapplying everything on top of a clean master fixed it.
>
> So it all works.
OK, whew. At least now I no longer _completely_ doubt the mental model I
have of UML VM :-)
> With some performance penalties compared to the old approach, but works.
I still find this odd though, I don't see what the flush would possibly
do in a new (mm host) process that's not achievable in arch_dup_mmap()?
OK, so let's see - arch_dup_mmap() is _earlier_ than the fork_handler,
because that only happens on the very first switch into the process.
This is only when it gets scheduled. So we'd be looking for something
that copy_process() changes in the MM after copy_mm() and before it can
get scheduled?
I guess we could even move the flush it into copy_thread(), which is a
simpler patch too, but it felt a bit wrong, since that's about the
(guest!) process, not the mm.
But basically I don't see anything there - fork syscall tail-calls
kernel_clone(), which doesn't really do anything with the result of
copy_process() except wake it up, and copy_process() doesn't really do
anything either?
johannes
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-25 14:44 ` Johannes Berg
@ 2023-09-25 15:20 ` Anton Ivanov
2023-09-26 12:16 ` Anton Ivanov
0 siblings, 1 reply; 19+ messages in thread
From: Anton Ivanov @ 2023-09-25 15:20 UTC (permalink / raw)
To: Johannes Berg, linux-um
On 25/09/2023 15:44, Johannes Berg wrote:
> On Mon, 2023-09-25 at 15:27 +0100, Anton Ivanov wrote:
>> On 25/09/2023 14:33, Johannes Berg wrote:
>>> On Mon, 2023-09-25 at 14:29 +0100, Anton Ivanov wrote:
>>>> I have rebased the preempt patch on top of these series.
>>>>
>>>> PREEMPT works with some performance decrease.
>>>>
>>>> VOLUNTARY deadlocks early in boot around the time it starts loading modules.
>>>>
>>>> non-preemptible deadlocks very early in boot.
>>>>
>>> Well I guess that means there's still some issue in here? Hmm.
>>>
>>> Now I don't understand anything anymore, I guess.
>> PEBKAC. The tree got corrupted somewhere during rebase. Reapplying everything on top of a clean master fixed it.
>>
>> So it all works.
> OK, whew. At least now I no longer _completely_ doubt the mental model I
> have of UML VM :-)
>
>> With some performance penalties compared to the old approach, but works.
> I still find this odd though, I don't see what the flush would possibly
> do in a new (mm host) process that's not achievable in arch_dup_mmap()?
>
> OK, so let's see - arch_dup_mmap() is _earlier_ than the fork_handler,
> because that only happens on the very first switch into the process.
> This is only when it gets scheduled. So we'd be looking for something
> that copy_process() changes in the MM after copy_mm() and before it can
> get scheduled?
>
> I guess we could even move the flush it into copy_thread(), which is a
> simpler patch too, but it felt a bit wrong, since that's about the
> (guest!) process, not the mm.
>
> But basically I don't see anything there - fork syscall tail-calls
> kernel_clone(), which doesn't really do anything with the result of
> copy_process() except wake it up, and copy_process() doesn't really do
> anything either?
I am continuing to dig through that and looking for voluntary preemption points in the process.
We have none of our own and the generic ones are not invoked - voluntary for all practical purposes does not differ from no-preempt.
If I have some suspects, I will post their mugshots to the list :)
> johannes
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-25 15:20 ` Anton Ivanov
@ 2023-09-26 12:16 ` Anton Ivanov
2023-09-26 12:38 ` Johannes Berg
0 siblings, 1 reply; 19+ messages in thread
From: Anton Ivanov @ 2023-09-26 12:16 UTC (permalink / raw)
To: Johannes Berg, linux-um
On 25/09/2023 16:20, Anton Ivanov wrote:
>
> On 25/09/2023 15:44, Johannes Berg wrote:
>> On Mon, 2023-09-25 at 15:27 +0100, Anton Ivanov wrote:
>>> On 25/09/2023 14:33, Johannes Berg wrote:
>>>> On Mon, 2023-09-25 at 14:29 +0100, Anton Ivanov wrote:
>>>>> I have rebased the preempt patch on top of these series.
>>>>>
>>>>> PREEMPT works with some performance decrease.
>>>>>
>>>>> VOLUNTARY deadlocks early in boot around the time it starts loading modules.
>>>>>
>>>>> non-preemptible deadlocks very early in boot.
>>>>>
>>>> Well I guess that means there's still some issue in here? Hmm.
>>>>
>>>> Now I don't understand anything anymore, I guess.
>>> PEBKAC. The tree got corrupted somewhere during rebase. Reapplying everything on top of a clean master fixed it.
>>>
>>> So it all works.
>> OK, whew. At least now I no longer _completely_ doubt the mental model I
>> have of UML VM :-)
>>
>>> With some performance penalties compared to the old approach, but works.
>> I still find this odd though, I don't see what the flush would possibly
>> do in a new (mm host) process that's not achievable in arch_dup_mmap()?
>>
>> OK, so let's see - arch_dup_mmap() is _earlier_ than the fork_handler,
>> because that only happens on the very first switch into the process.
>> This is only when it gets scheduled. So we'd be looking for something
>> that copy_process() changes in the MM after copy_mm() and before it can
>> get scheduled?
>>
>> I guess we could even move the flush it into copy_thread(), which is a
>> simpler patch too, but it felt a bit wrong, since that's about the
>> (guest!) process, not the mm.
>>
>> But basically I don't see anything there - fork syscall tail-calls
>> kernel_clone(), which doesn't really do anything with the result of
>> copy_process() except wake it up, and copy_process() doesn't really do
>> anything either?
>
> I am continuing to dig through that and looking for voluntary preemption points in the process.
>
> We have none of our own and the generic ones are not invoked - voluntary for all practical purposes does not differ from no-preempt.
>
> If I have some suspects, I will post their mugshots to the list :)
For the time being it is mostly negative :)
1. The performance after the mm patch is down. By 30-40% on my standard bench.
2. The preemption patches work fine on top (all 3 cases). The performance difference stays.
3. We do not have anything of value to add in term of cond_resched() to the drivers :(
Most drivers are fairly simplistic with no safe points to add this.
4. The upper layer resched points are not enough.
F.E. -EAGAIN from block, retries, etc should all hit paths in the upper layer which cause a
cond_resched(). They are not noticeable. VOLUNTARY behaves nearly identical to non-preemptive.
5. There are a couple of places where an if (condition) schedule() should probably be replaced
with cond_resched(). F.E. interrupt_end(). While it looks more appropriate, the performance
effect is nil.
6. Do we still need force_flush_all() in the arch_dup_mmap()? This works with a non-forced tlb flush
using flush_tlb_mm(mm);
7. In all cases, UML is doing something silly.
The CPU usage while doing find -type f -exec cat {} > /dev/null measured from outside in non-preemptive and
PREEMPT_VOLUNTARY stays around 8-15%. The UML takes a sabbatical for the remaining 85 instead of actually
doing work. PREEMPT is slightly better at 60, but still far from 100%. It just keeps going into idle and I
cannot understand why.
8. All in all I do not have an obvious suspect for the performance difference. No change candidates
either. I also do not have any ideas why it ends up in idle instead of doing work.
>
>> johannes
>>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-26 12:16 ` Anton Ivanov
@ 2023-09-26 12:38 ` Johannes Berg
2023-09-26 13:04 ` Anton Ivanov
2023-09-27 9:52 ` Benjamin Berg
0 siblings, 2 replies; 19+ messages in thread
From: Johannes Berg @ 2023-09-26 12:38 UTC (permalink / raw)
To: Anton Ivanov, linux-um
On Tue, 2023-09-26 at 13:16 +0100, Anton Ivanov wrote:
>
> For the time being it is mostly negative :)
Oh well :)
> 1. The performance after the mm patch is down. By 30-40% on my standard bench.
For the record, you mean this three-patch series that we're discussing
in the thread of?
Btw, Benjamin realized that MADV_DONTFORK is broken in UML, precisely
_because_ we fork/copy the whole mm process and then try to fix it up.
But we can only fix up things that actually have VMAs, and of course
there are no VMAs with VM_DONTCOPY (set by MADV_DONTFORK) in the new mm
after fork.
To fix this, really we should either
1. Start from scratch, without copying, which my other patch [1] did.
[1] https://lore.kernel.org/all/20230922131638.2c57ec713d1c.Id11dff4b349e6a8f0136bb6bb09f6e01a80befbb@changeid/
But of course that's more expensive because we now have to page-fault
everything in the new process, and page faults are expensive.
2. Compare the new mm and the old mm, which requires putting it into
arch_dup_mmap() like these patches here - where I'm not sure I
understand at all why they cause a perf regression - and remove the
VMAs that are marked VM_DONTCOPY in the old one.
To be honest I don't really like _either_ of these approaches, nor the
current "fork the process" approach that UML takes. It's very magic, and
very much works around how Linux works.
Remember that basically the mm process contents should match the page
tables in the VMAs; but this is decidedly not true where fork() is
involved, because while the VMAs are copied, most of the page tables are
_not_ copied. Thus, we have a situation where after fork we don't take
page faults in UML that we would take in a normal system (this part is
good for performance), and I believe also vice versa, which would then
perhaps explain the flush_tlb_page() in handle_page_fault(), because
honestly I don't otherwise have an explanation for it.
I think the better approach for correctness and integration into the
kernel would be to actually admit that UML is special because page
faults are so expensive, and
* start with a fresh mm process every time
* have vma_needs_copy() return true
* completely fill the mappings according to only the new mm's VMAs
in arch_dup_mmap() or perhaps later
I don't know how that'd behave wrt. performance, though it likely cannot
be better than with these patches, but at least it'd be more correct,
and more obviously correct too, for starters, because then the actual
mappings in the UML mm process would actually reflect the PTEs that
Linux knows about.
> 2. The preemption patches work fine on top (all 3 cases). The performance difference stays.
OK.
> 3. We do not have anything of value to add in term of cond_resched() to the drivers :(
> Most drivers are fairly simplistic with no safe points to add this.
Yeah, not surprised by this.
> 6. Do we still need force_flush_all() in the arch_dup_mmap()? This works with a non-forced tlb flush
> using flush_tlb_mm(mm);
Maybe not, does it make a difference though?
> 7. In all cases, UML is doing something silly.
> The CPU usage while doing find -type f -exec cat {} > /dev/null measured from outside in non-preemptive and
> PREEMPT_VOLUNTARY stays around 8-15%. The UML takes a sabbatical for the remaining 85 instead of actually
> doing work. PREEMPT is slightly better at 60, but still far from 100%. It just keeps going into idle and I
> cannot understand why.
Is it just waiting for IO?
johannes
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-26 12:38 ` Johannes Berg
@ 2023-09-26 13:04 ` Anton Ivanov
2023-09-27 9:52 ` Benjamin Berg
1 sibling, 0 replies; 19+ messages in thread
From: Anton Ivanov @ 2023-09-26 13:04 UTC (permalink / raw)
To: Johannes Berg, linux-um
On 26/09/2023 13:38, Johannes Berg wrote:
> On Tue, 2023-09-26 at 13:16 +0100, Anton Ivanov wrote:
>>
>> For the time being it is mostly negative :)
>
> Oh well :)
>
>> 1. The performance after the mm patch is down. By 30-40% on my standard bench.
>
> For the record, you mean this three-patch series that we're discussing
> in the thread of?
Yes. It has no stability issues on its own as well as with the PREEMPT patch on top.
>
>
> Btw, Benjamin realized that MADV_DONTFORK is broken in UML, precisely
> _because_ we fork/copy the whole mm process and then try to fix it up.
> But we can only fix up things that actually have VMAs, and of course
> there are no VMAs with VM_DONTCOPY (set by MADV_DONTFORK) in the new mm
> after fork.
>
> To fix this, really we should either
>
> 1. Start from scratch, without copying, which my other patch [1] did.
>
> [1] https://lore.kernel.org/all/20230922131638.2c57ec713d1c.Id11dff4b349e6a8f0136bb6bb09f6e01a80befbb@changeid/
>
> But of course that's more expensive because we now have to page-fault
> everything in the new process, and page faults are expensive.
>
> 2. Compare the new mm and the old mm, which requires putting it into
> arch_dup_mmap() like these patches here - where I'm not sure I
> understand at all why they cause a perf regression - and remove the
> VMAs that are marked VM_DONTCOPY in the old one.
>
>
> To be honest I don't really like _either_ of these approaches, nor the
> current "fork the process" approach that UML takes. It's very magic, and
> very much works around how Linux works.
+1
>
> Remember that basically the mm process contents should match the page
> tables in the VMAs; but this is decidedly not true where fork() is
> involved, because while the VMAs are copied, most of the page tables are
> _not_ copied. Thus, we have a situation where after fork we don't take
> page faults in UML that we would take in a normal system (this part is
> good for performance), and I believe also vice versa, which would then
> perhaps explain the flush_tlb_page() in handle_page_fault(), because
> honestly I don't otherwise have an explanation for it.
>
>
> I think the better approach for correctness and integration into the
> kernel would be to actually admit that UML is special because page
> faults are so expensive, and
>
> * start with a fresh mm process every time
> * have vma_needs_copy() return true
> * completely fill the mappings according to only the new mm's VMAs
> in arch_dup_mmap() or perhaps later
>
> I don't know how that'd behave wrt. performance, though it likely cannot
> be better than with these patches, but at least it'd be more correct,
> and more obviously correct too, for starters, because then the actual
> mappings in the UML mm process would actually reflect the PTEs that
> Linux knows about.
We can try that.
>
>
>> 2. The preemption patches work fine on top (all 3 cases). The performance difference stays.
>
> OK.
>
>> 3. We do not have anything of value to add in term of cond_resched() to the drivers :(
>> Most drivers are fairly simplistic with no safe points to add this.
>
> Yeah, not surprised by this.
>
>> 6. Do we still need force_flush_all() in the arch_dup_mmap()? This works with a non-forced tlb flush
>> using flush_tlb_mm(mm);
>
> Maybe not, does it make a difference though?
Nope. Same numbers in both cases.
>
>> 7. In all cases, UML is doing something silly.
>> The CPU usage while doing find -type f -exec cat {} > /dev/null measured from outside in non-preemptive and
>> PREEMPT_VOLUNTARY stays around 8-15%. The UML takes a sabbatical for the remaining 85 instead of actually
>> doing work. PREEMPT is slightly better at 60, but still far from 100%. It just keeps going into idle and I
>> cannot understand why.
>
> Is it just waiting for IO?
Nope. Nearly all I see on strace is wait4 and PTRACE. The epoll_waits are few and far between.
The bottleneck is mm and vm, not IO :(
>
> johannes
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-26 12:38 ` Johannes Berg
2023-09-26 13:04 ` Anton Ivanov
@ 2023-09-27 9:52 ` Benjamin Berg
2023-09-27 9:59 ` Anton Ivanov
2024-01-17 17:17 ` Benjamin Berg
1 sibling, 2 replies; 19+ messages in thread
From: Benjamin Berg @ 2023-09-27 9:52 UTC (permalink / raw)
To: Johannes Berg, Anton Ivanov, linux-um
Hi,
On Tue, 2023-09-26 at 14:38 +0200, Johannes Berg wrote:
> [SNIP]
> 1. Start from scratch, without copying, which my other patch [1] did.
I really think we should go ahead with that approach. Then follow up
with optimizations.
> [SNIP]
>
> I think the better approach for correctness and integration into the
> kernel would be to actually admit that UML is special because page
> faults are so expensive, and
>
> * start with a fresh mm process every time
> * have vma_needs_copy() return true
> * completely fill the mappings according to only the new mm's VMAs
> in arch_dup_mmap() or perhaps later
>
> I don't know how that'd behave wrt. performance, though it likely cannot
> be better than with these patches, but at least it'd be more correct,
> and more obviously correct too, for starters, because then the actual
> mappings in the UML mm process would actually reflect the PTEs that
> Linux knows about.
Yes, performance may degrade, but the implementation should be correct
in the first place. Note that even though we looked at it (and e.g.
found that MMAP_DONTFORK is incorrect), we have not figured out why the
first approach is slower currently as everything interesting should be
getting unmapped by the force_flush_all.
Once we are there, we can look for optimizations. The fundamental
problem is that page faults (even minor ones) are extremely expensive
for us.
Just throwing out ideas on what we could do:
1. SECCOMP as that reduces the amount of context switches.
(Yes, I know I should resubmit the patchset)
2. Maybe we can disable/cripple page access tracking? If we assume
initially mark all pages as accessed by userspace (i.e.
pte_mkyoung), then we avoid a minor page fault on first access.
Doing that will mess with page eviction though.
3. Do DAX (direct_access) for files. i.e. mmap files directly in the
host kernel rather than through UM.
With a hostfs like file system, one should be able to add an
intermediate block device that maps host files to physical pages,
then do DAX in the FS.
For disk images, the existing iomem infrastructure should be
usable, this should work with any DAX enabled filesystems (ext2,
ext4, xfs, virtiofs, erofs).
Benjamin
>
> > 2. The preemption patches work fine on top (all 3 cases). The
> > performance difference stays.
>
> OK.
>
> > 3. We do not have anything of value to add in term of
> > cond_resched() to the drivers :(
> > Most drivers are fairly simplistic with no safe points to add this.
>
> Yeah, not surprised by this.
>
> > 6. Do we still need force_flush_all() in the arch_dup_mmap()? This
> > works with a non-forced tlb flush
> > using flush_tlb_mm(mm);
>
> Maybe not, does it make a difference though?
>
> > 7. In all cases, UML is doing something silly.
> > The CPU usage while doing find -type f -exec cat {} > /dev/null
> > measured from outside in non-preemptive and
> > PREEMPT_VOLUNTARY stays around 8-15%. The UML takes a sabbatical
> > for the remaining 85 instead of actually
> > doing work. PREEMPT is slightly better at 60, but still far from
> > 100%. It just keeps going into idle and I
> > cannot understand why.
>
> Is it just waiting for IO?
>
> johannes
>
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
>
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-27 9:52 ` Benjamin Berg
@ 2023-09-27 9:59 ` Anton Ivanov
2023-09-27 10:42 ` Benjamin Berg
2024-01-17 17:17 ` Benjamin Berg
1 sibling, 1 reply; 19+ messages in thread
From: Anton Ivanov @ 2023-09-27 9:59 UTC (permalink / raw)
To: Benjamin Berg, Johannes Berg, linux-um
On 27/09/2023 10:52, Benjamin Berg wrote:
> Hi,
>
> On Tue, 2023-09-26 at 14:38 +0200, Johannes Berg wrote:
>> [SNIP]
>> 1. Start from scratch, without copying, which my other patch [1] did.
>
> I really think we should go ahead with that approach. Then follow up
> with optimizations.
+1
>
>> [SNIP]
>>
>> I think the better approach for correctness and integration into the
>> kernel would be to actually admit that UML is special because page
>> faults are so expensive, and
>>
>> * start with a fresh mm process every time
>> * have vma_needs_copy() return true
>> * completely fill the mappings according to only the new mm's VMAs
>> in arch_dup_mmap() or perhaps later
>>
>> I don't know how that'd behave wrt. performance, though it likely cannot
>> be better than with these patches, but at least it'd be more correct,
>> and more obviously correct too, for starters, because then the actual
>> mappings in the UML mm process would actually reflect the PTEs that
>> Linux knows about.
>
> Yes, performance may degrade, but the implementation should be correct
> in the first place. Note that even though we looked at it (and e.g.
> found that MMAP_DONTFORK is incorrect), we have not figured out why the
> first approach is slower currently as everything interesting should be
> getting unmapped by the force_flush_all.
>
> Once we are there, we can look for optimizations. The fundamental
> problem is that page faults (even minor ones) are extremely expensive
> for us.
>
> Just throwing out ideas on what we could do:
> 1. SECCOMP as that reduces the amount of context switches.
> (Yes, I know I should resubmit the patchset)
Actually... YES, YES and YES.
I was just looking at all the workaround which are in place to prevent
guest processes doing a syscall on the host. If this is prohibited at
a higher level we should get quite a boost as all these PTRACE_PEEKs
will become unnecessary.
> 2. Maybe we can disable/cripple page access tracking? If we assume
> initially mark all pages as accessed by userspace (i.e.
> pte_mkyoung), then we avoid a minor page fault on first access.
> Doing that will mess with page eviction though.
> 3. Do DAX (direct_access) for files. i.e. mmap files directly in the
> host kernel rather than through UM.
> With a hostfs like file system, one should be able to add an
> intermediate block device that maps host files to physical pages,
> then do DAX in the FS.
> For disk images, the existing iomem infrastructure should be
> usable, this should work with any DAX enabled filesystems (ext2,
> ext4, xfs, virtiofs, erofs).
I had some plans to do a ubd gen 2 which uses mmap and/or this. They are
presently way on the backburner. We can do some of that once we push
the new VM changes.
>
> Benjamin
>
>>
>>> 2. The preemption patches work fine on top (all 3 cases). The
>>> performance difference stays.
>>
>> OK.
>>
>>> 3. We do not have anything of value to add in term of
>>> cond_resched() to the drivers :(
>>> Most drivers are fairly simplistic with no safe points to add this.
>>
>> Yeah, not surprised by this.
>>
>>> 6. Do we still need force_flush_all() in the arch_dup_mmap()? This
>>> works with a non-forced tlb flush
>>> using flush_tlb_mm(mm);
>>
>> Maybe not, does it make a difference though?
>>
>>> 7. In all cases, UML is doing something silly.
>>> The CPU usage while doing find -type f -exec cat {} > /dev/null
>>> measured from outside in non-preemptive and
>>> PREEMPT_VOLUNTARY stays around 8-15%. The UML takes a sabbatical
>>> for the remaining 85 instead of actually
>>> doing work. PREEMPT is slightly better at 60, but still far from
>>> 100%. It just keeps going into idle and I
>>> cannot understand why.
>>
>> Is it just waiting for IO?
>>
>> johannes
>>
>> _______________________________________________
>> linux-um mailing list
>> linux-um@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-um
>>
>
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-27 9:59 ` Anton Ivanov
@ 2023-09-27 10:42 ` Benjamin Berg
0 siblings, 0 replies; 19+ messages in thread
From: Benjamin Berg @ 2023-09-27 10:42 UTC (permalink / raw)
To: Anton Ivanov, Johannes Berg, linux-um
Hi,
On Wed, 2023-09-27 at 10:59 +0100, Anton Ivanov wrote:
> [SNIP]
>
> I was just looking at all the workaround which are in place to
> prevent guest processes doing a syscall on the host. If this is
> prohibited at a higher level we should get quite a boost as all these
> PTRACE_PEEKs will become unnecessary.
Are you maybe looking at the fallback code if "local_using_sysemu" is
not set? That code should be dropped either way, we can assume
PTRACE_SYSEMU support these days, so the code in e.g. handle_trap can
be removed. I had a patch for that already.
Benjamin
>
> > 2. Maybe we can disable/cripple page access tracking? If we
> > assume
> > initially mark all pages as accessed by userspace (i.e.
> > pte_mkyoung), then we avoid a minor page fault on first
> > access.
> > Doing that will mess with page eviction though.
> > 3. Do DAX (direct_access) for files. i.e. mmap files directly
> > in the
> > host kernel rather than through UM.
> > With a hostfs like file system, one should be able to add an
> > intermediate block device that maps host files to physical
> > pages,
> > then do DAX in the FS.
> > For disk images, the existing iomem infrastructure should be
> > usable, this should work with any DAX enabled filesystems
> > (ext2,
> > ext4, xfs, virtiofs, erofs).
>
> I had some plans to do a ubd gen 2 which uses mmap and/or this. They
> are
> presently way on the backburner. We can do some of that once we push
> the new VM changes.
>
> >
> > Benjamin
> >
> > >
> > > > 2. The preemption patches work fine on top (all 3 cases). The
> > > > performance difference stays.
> > >
> > > OK.
> > >
> > > > 3. We do not have anything of value to add in term of
> > > > cond_resched() to the drivers :(
> > > > Most drivers are fairly simplistic with no safe points to add
> > > > this.
> > >
> > > Yeah, not surprised by this.
> > >
> > > > 6. Do we still need force_flush_all() in the arch_dup_mmap()?
> > > > This
> > > > works with a non-forced tlb flush
> > > > using flush_tlb_mm(mm);
> > >
> > > Maybe not, does it make a difference though?
> > >
> > > > 7. In all cases, UML is doing something silly.
> > > > The CPU usage while doing find -type f -exec cat {} > /dev/null
> > > > measured from outside in non-preemptive and
> > > > PREEMPT_VOLUNTARY stays around 8-15%. The UML takes a
> > > > sabbatical
> > > > for the remaining 85 instead of actually
> > > > doing work. PREEMPT is slightly better at 60, but still far
> > > > from
> > > > 100%. It just keeps going into idle and I
> > > > cannot understand why.
> > >
> > > Is it just waiting for IO?
> > >
> > > johannes
> > >
> > > _______________________________________________
> > > linux-um mailing list
> > > linux-um@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-um
> > >
> >
> >
>
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2023-09-27 9:52 ` Benjamin Berg
2023-09-27 9:59 ` Anton Ivanov
@ 2024-01-17 17:17 ` Benjamin Berg
2024-01-17 19:45 ` Anton Ivanov
1 sibling, 1 reply; 19+ messages in thread
From: Benjamin Berg @ 2024-01-17 17:17 UTC (permalink / raw)
To: Johannes Berg, Anton Ivanov, linux-um
Hi,
On Wed, 2023-09-27 at 11:52 +0200, Benjamin Berg wrote:
> [SNIP]
> Once we are there, we can look for optimizations. The fundamental
> problem is that page faults (even minor ones) are extremely expensive
> for us.
>
> Just throwing out ideas on what we could do:
> 1. SECCOMP as that reduces the amount of context switches.
> (Yes, I know I should resubmit the patchset)
> 2. Maybe we can disable/cripple page access tracking? If we assume
> initially mark all pages as accessed by userspace (i.e.
> pte_mkyoung), then we avoid a minor page fault on first access.
> Doing that will mess with page eviction though.
> 3. Do DAX (direct_access) for files. i.e. mmap files directly in the
> host kernel rather than through UM.
> With a hostfs like file system, one should be able to add an
> intermediate block device that maps host files to physical pages,
> then do DAX in the FS.
> For disk images, the existing iomem infrastructure should be
> usable, this should work with any DAX enabled filesystems (ext2,
> ext4, xfs, virtiofs, erofs).
So, I experimented quite a bit over Christmas (including getting DAX to
work with virtiofs). At the end of all this my conclusion is that
insufficient page table synchronization is our main problem.
Basically, right now we rely on the flush_tlb_* functions from the
kernel, but these are only called when TLB entries are removed, *not*
when new PTEs are added (there is also update_mmu_cache, but it isn't
enough either). Effectively this means that new page table entries will
often only be synced because the userspace code runs into an
unnecessary segfright now we rely on the flush_tlb_* functions from the
kernel, but these are only called when TLB entries are removed, *not*
when new PTEs are added (there is also update_mmu_cache, but it isn't
enough either). Effectively this means that new page table entries will
often only be synced because the userspace code runs into an
unnecessary segfaultault.
Really, what we need is a set_pte_at() implementation that marks the
memory range for synchronization. Then we can make sure we sync it
before switching to the userspace process (the equivalent of running
flush_tlb_mm_range right now).
I think we should:
* Rewrite the userspace syscall code
- Support delaying the execution of syscalls
- Only support mmap/munmap/mprotect and LDT
- Do simple compression of consecutive syscalls here
- Drop the hand-written assembler
* Improve the tlb.c code
- remove the HVC abstraction
- never force immediate syscall execution
* Let set_pte_at() track which memory ranges that need syncing
* At that point we should be able to:
- drop copy_context_skas0
- make flush_tlb_* no-ops
- drop flush_tlb_page from handle_page_fault
- move unmap() from flush_thread to init_new_context
(or do it as part of start_userspace)
So, I did try this using nasty hacks and IIRC one of my runs was going
from 21s to 16s and another from 63s to 56s. Which seems like a nice
improvement.
Benjamin
PS: As for DAX, it doesn't really seem to help performance. It didn't
seem to lower the amount of page faults in UML. And, from my
perspective, it isn't really worth just for the memory sharing.
PPS: dirty/young tracking seemed to be only cause a small amount of
page faults in the grand scheme. So probably not something worth
following up on.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2024-01-17 17:17 ` Benjamin Berg
@ 2024-01-17 19:45 ` Anton Ivanov
2024-01-17 19:54 ` Benjamin Berg
0 siblings, 1 reply; 19+ messages in thread
From: Anton Ivanov @ 2024-01-17 19:45 UTC (permalink / raw)
To: Benjamin Berg, Johannes Berg, linux-um
On 17/01/2024 17:17, Benjamin Berg wrote:
> Hi,
>
> On Wed, 2023-09-27 at 11:52 +0200, Benjamin Berg wrote:
>> [SNIP]
>> Once we are there, we can look for optimizations. The fundamental
>> problem is that page faults (even minor ones) are extremely expensive
>> for us.
>>
>> Just throwing out ideas on what we could do:
>> 1. SECCOMP as that reduces the amount of context switches.
>> (Yes, I know I should resubmit the patchset)
>> 2. Maybe we can disable/cripple page access tracking? If we assume
>> initially mark all pages as accessed by userspace (i.e.
>> pte_mkyoung), then we avoid a minor page fault on first access.
>> Doing that will mess with page eviction though.
>> 3. Do DAX (direct_access) for files. i.e. mmap files directly in the
>> host kernel rather than through UM.
>> With a hostfs like file system, one should be able to add an
>> intermediate block device that maps host files to physical pages,
>> then do DAX in the FS.
>> For disk images, the existing iomem infrastructure should be
>> usable, this should work with any DAX enabled filesystems (ext2,
>> ext4, xfs, virtiofs, erofs).
>
> So, I experimented quite a bit over Christmas (including getting DAX to
> work with virtiofs). At the end of all this my conclusion is that
> insufficient page table synchronization is our main problem.
>
> Basically, right now we rely on the flush_tlb_* functions from the
> kernel, but these are only called when TLB entries are removed, *not*
> when new PTEs are added (there is also update_mmu_cache, but it isn't
> enough either). Effectively this means that new page table entries will
> often only be synced because the userspace code runs into an
> unnecessary segfright now we rely on the flush_tlb_* functions from the
> kernel, but these are only called when TLB entries are removed, *not*
> when new PTEs are added (there is also update_mmu_cache, but it isn't
> enough either). Effectively this means that new page table entries will
> often only be synced because the userspace code runs into an
> unnecessary segfaultault.
>
> Really, what we need is a set_pte_at() implementation that marks the
> memory range for synchronization. Then we can make sure we sync it
> before switching to the userspace process (the equivalent of running
> flush_tlb_mm_range right now).
>
> I think we should:
> * Rewrite the userspace syscall code
> - Support delaying the execution of syscalls
> - Only support mmap/munmap/mprotect and LDT
> - Do simple compression of consecutive syscalls here
> - Drop the hand-written assembler
> * Improve the tlb.c code
> - remove the HVC abstraction
Cool. That was not working particularly well. I tried to improve it a
few times, but ripping it out and replacing it is probably a better idea.
> - never force immediate syscall execution
> * Let set_pte_at() track which memory ranges that need syncing
> * At that point we should be able to:
> - drop copy_context_skas0
> - make flush_tlb_* no-ops
> - drop flush_tlb_page from handle_page_fault
> - move unmap() from flush_thread to init_new_context
> (or do it as part of start_userspace)
>
> So, I did try this using nasty hacks and IIRC one of my runs was going
> from 21s to 16s and another from 63s to 56s. Which seems like a nice
> improvement.
Excellent. I assume you were using hostfs as usual, right? If so, the
difference is likely to be even more noticeable on ubd.
>
> Benjamin
>
>
> PS: As for DAX, it doesn't really seem to help performance. It didn't
> seem to lower the amount of page faults in UML. And, from my
> perspective, it isn't really worth just for the memory sharing.
>
> PPS: dirty/young tracking seemed to be only cause a small amount of
> page faults in the grand scheme. So probably not something worth
> following up on.
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] um: clean up mm creation - another attempt
2024-01-17 19:45 ` Anton Ivanov
@ 2024-01-17 19:54 ` Benjamin Berg
0 siblings, 0 replies; 19+ messages in thread
From: Benjamin Berg @ 2024-01-17 19:54 UTC (permalink / raw)
To: Anton Ivanov, Johannes Berg, linux-um
On Wed, 2024-01-17 at 19:45 +0000, Anton Ivanov wrote:
> On 17/01/2024 17:17, Benjamin Berg wrote:
> > Hi,
> >
> > On Wed, 2023-09-27 at 11:52 +0200, Benjamin Berg wrote:
> > > [SNIP]
> > > Once we are there, we can look for optimizations. The fundamental
> > > problem is that page faults (even minor ones) are extremely expensive
> > > for us.
> > >
> > > Just throwing out ideas on what we could do:
> > > 1. SECCOMP as that reduces the amount of context switches.
> > > (Yes, I know I should resubmit the patchset)
> > > 2. Maybe we can disable/cripple page access tracking? If we assume
> > > initially mark all pages as accessed by userspace (i.e.
> > > pte_mkyoung), then we avoid a minor page fault on first access.
> > > Doing that will mess with page eviction though.
> > > 3. Do DAX (direct_access) for files. i.e. mmap files directly in the
> > > host kernel rather than through UM.
> > > With a hostfs like file system, one should be able to add an
> > > intermediate block device that maps host files to physical pages,
> > > then do DAX in the FS.
> > > For disk images, the existing iomem infrastructure should be
> > > usable, this should work with any DAX enabled filesystems (ext2,
> > > ext4, xfs, virtiofs, erofs).
> >
> > So, I experimented quite a bit over Christmas (including getting DAX to
> > work with virtiofs). At the end of all this my conclusion is that
> > insufficient page table synchronization is our main problem.
> >
> > Basically, right now we rely on the flush_tlb_* functions from the
> > kernel, but these are only called when TLB entries are removed, *not*
> > when new PTEs are added (there is also update_mmu_cache, but it isn't
> > enough either). Effectively this means that new page table entries will
> > often only be synced because the userspace code runs into an
> > unnecessary segfright now we rely on the flush_tlb_* functions from the
> > kernel, but these are only called when TLB entries are removed, *not*
> > when new PTEs are added (there is also update_mmu_cache, but it isn't
> > enough either). Effectively this means that new page table entries will
> > often only be synced because the userspace code runs into an
> > unnecessary segfaultault.
> >
> > Really, what we need is a set_pte_at() implementation that marks the
> > memory range for synchronization. Then we can make sure we sync it
> > before switching to the userspace process (the equivalent of running
> > flush_tlb_mm_range right now).
> >
> > I think we should:
> > * Rewrite the userspace syscall code
> > - Support delaying the execution of syscalls
> > - Only support mmap/munmap/mprotect and LDT
> > - Do simple compression of consecutive syscalls here
> > - Drop the hand-written assembler
> > * Improve the tlb.c code
> > - remove the HVC abstraction
>
> Cool. That was not working particularly well. I tried to improve it a
> few times, but ripping it out and replacing it is probably a better idea.
Hm, now I realise that we still want mmap() syscall compression for the
kernel itself in tlb.c.
> > - never force immediate syscall execution
> > * Let set_pte_at() track which memory ranges that need syncing
> > * At that point we should be able to:
> > - drop copy_context_skas0
> > - make flush_tlb_* no-ops
> > - drop flush_tlb_page from handle_page_fault
> > - move unmap() from flush_thread to init_new_context
> > (or do it as part of start_userspace)
> >
> > So, I did try this using nasty hacks and IIRC one of my runs was going
> > from 21s to 16s and another from 63s to 56s. Which seems like a nice
> > improvement.
>
> Excellent. I assume you were using hostfs as usual, right? If so, the
> difference is likely to be even more noticeable on ubd.
Yes, I was mostly testing hostfs. Initially also virtiofs with DAX, but
I went back as that didn't result in a pagefault count improvement once
I made some other adjustments.
Benjamin
>
> >
> > Benjamin
> >
> >
> > PS: As for DAX, it doesn't really seem to help performance. It didn't
> > seem to lower the amount of page faults in UML. And, from my
> > perspective, it isn't really worth just for the memory sharing.
> >
> > PPS: dirty/young tracking seemed to be only cause a small amount of
> > page faults in the grand scheme. So probably not something worth
> > following up on.
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread