Messages in this thread Patch in this message | | | From | Vitaly Kuznetsov <> | Subject | Re: [lkp-robot] [x86/mm] 9e52fc2b50: will-it-scale.per_thread_ops -16% regression | Date | Mon, 20 Nov 2017 17:58:17 +0100 |
| |
Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> But adding such complexity to the code would require a good > justification, of course.
Sorry for necroposting, I got distracted :-(
I think I was able to reproduce the reported regression. The reproducer is dead simple, just several threads doing malloc(128Mb)/free().
#include <pthread.h> #include <stdio.h> #include <stdlib.h>
#define nthreads 16 #define nrounds 10000 #define alloc_size 128*1024*1024 /*128Mb*/
void *threadf(void *ptr) { void *addr; int i;
for (i = 0; i < nrounds; i++) { addr = malloc(alloc_size); if (!addr) { fprintf(stderr, "malloc failed\n"); exit(1); } free(addr); } }
int main(int argc, char *argv[]) { pthread_t thr[nthreads]; int i;
for (i = 0; i < nthreads; i++) { if (pthread_create(&thr[i], NULL, threadf, NULL)) { fprintf(stderr, "pthread_create failed\n"); exit(1); } }
for (i = 0; i < nthreads; i++) { if (pthread_join(thr[i], NULL)) { fprintf(stderr, "pthread_join failed\n"); exit(1); } }
return 0; }
The average result on a 16 core host for me is:
With HAVE_RCU_TABLE_FREE (what we have now): real 0m10.571s user 0m0.678s sys 0m12.813s
Without HAVE_RCU_TABLE_FREE (what we had pre-patch): real 0m9.976s user 0m0.824s sys 0m10.960s
I did some investigation and I *think* this is what's going on. We have the following call chain:
do_munmap() unmap_region() free_pgtables() tlb_finish_mmu() arch_tlb_finish_mmu() tlb_flush_mmu() tlb_flush_mmu_tlbonly() tlb_flush() <- IPIs on bare metal tlb_table_flush() <- this is added when CONFIG_HAVE_RCU_TABLE_FREE
tlb_table_flush() does call_rcu_sched() to free the batch but the problem is that the batch is almost empty -- usually it has just one entry in it (for the above example).
The following dirty hack:
--- a/mm/memory.c +++ b/mm/memory.c @@ -367,7 +367,10 @@ void tlb_table_flush(struct mmu_gather *tlb) struct mmu_table_batch **batch = &tlb->batch; if (*batch) { - call_rcu_sched(&(*batch)->rcu, tlb_remove_table_rcu); + if (pv_mmu_ops.flush_tlb_others != native_flush_tlb_others) + call_rcu_sched(&(*batch)->rcu, tlb_remove_table_rcu); + else + tlb_remove_table_rcu(&(*batch)->rcu); *batch = NULL; } } seems to solve the issue. However, I'm having troubles trying to understand what would be the best move here. In case we think this use-case needs addressing I can suggest we employ static_keys and switch between rcu/non-rcu table free mechanisms for x86 on boot.
I'd be grateful for any thoughts/suggestions on this. Thanks!
-- Vitaly
| |