[RFC] posix-timers: RCU conversion

[RFC] posix-timers: RCU conversion

[Eric Dumazet]

Le lundi 21 mars 2011 à 23:57 +0545, Ben Nagy a écrit :
> On Mon, Mar 21, 2011 at 10:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le lundi 21 mars 2011 à 19:02 +0200, Avi Kivity a écrit :
> >
> >> Any ideas on how to fix it?  We could pre-allocate IDs and batch them in
> >> per-cpu caches, but it seems like a lot of work.
> >>
> >
> > Hmm, I dont know what syscalls kvm do, but even a timer_gettime() has to
> > lock this idr_lock.
> >
> > Sounds crazy, and should be fixed in kernel code, not kvm ;)
> 
> Hi,
> 
> I'll need to work out a way I can make the perf.data available
> (~100M), but here's the summary
> 
> http://paste.ubuntu.com/583425/
> 
> And here's the summary of the summary
> 
> # Overhead          Command         Shared Object
>                 Symbol
> # ........  ...............  ....................
> ..........................................
> #
>     71.86%              kvm  [kernel.kallsyms]     [k] __ticket_spin_lock
>                         |
>                         --- __ticket_spin_lock
>                            |
>                            |--54.19%-- default_spin_lock_flags
>                            |          _raw_spin_lock_irqsave
>                            |          |
>                            |           --54.14%-- __lock_timer
>                            |                     |
>                            |                     |--31.92%-- sys_timer_gettime
>                            |                     |          system_call_fastpath
>                            |                     |
>                            |                      --22.22%-- sys_timer_settime
>                            |                                system_call_fastpath
>                            |
>                            |--15.66%-- _raw_spin_lock
> [...]
> 
> Which I guess seems to match what Eric just said.
> 
> I'll post a link to the full data tomorrow. Many thanks for the help
> so far. If it's a kernel scaling limit then I guess we just wait until
> the kernel gets better. :S I'll check it out with a linux guest
> tomorrow anyway.

Here is a quick & dirty patch to lower idr_lock use. You could try it
eventually ;)

Thanks

[RFC] posix-timers: RCU conversion

Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
a single spinlock (idr_lock) in posix-timers code.

Even on a 16 cpu machine (2x4x2), a single test can show 98% of cpu time
used in ticket_spin_lock

Switching to RCU should be quite easy, IDR being already RCU ready.

idr_lock should be locked only before an insert/delete, not a find.

Reported-by: Ben Nagy <ben@iagu.net>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Avi Kivity <avi@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Richard Cochran <richard.cochran@omicron.at>
---
 include/linux/posix-timers.h |    1 +
 kernel/posix-timers.c        |   26 ++++++++++++++------------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index d51243a..5dc27ca 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -81,6 +81,7 @@ struct k_itimer {
 			unsigned long expires;
 		} mmtimer;
 	} it;
+	struct rcu_head rcu;
 };
 
 struct k_clock {
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 4c01249..e2a823a 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -491,6 +491,11 @@ static struct k_itimer * alloc_posix_timer(void)
 	return tmr;
 }
 
+static void k_itimer_rcu_free(struct rcu_head *head)
+{
+	kmem_cache_free(posix_timers_cache, container_of(head, struct k_itimer, rcu));
+}
+
 #define IT_ID_SET	1
 #define IT_ID_NOT_SET	0
 static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
@@ -499,11 +504,12 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 		unsigned long flags;
 		spin_lock_irqsave(&idr_lock, flags);
 		idr_remove(&posix_timers_id, tmr->it_id);
+		tmr->it_id = -1;
 		spin_unlock_irqrestore(&idr_lock, flags);
 	}
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
-	kmem_cache_free(posix_timers_cache, tmr);
+	call_rcu(&tmr->rcu, k_itimer_rcu_free);
 }
 
 static struct k_clock *clockid_to_kclock(const clockid_t id)
@@ -631,22 +637,18 @@ out:
 static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
 {
 	struct k_itimer *timr;
-	/*
-	 * Watch out here.  We do a irqsave on the idr_lock and pass the
-	 * flags part over to the timer lock.  Must not let interrupts in
-	 * while we are moving the lock.
-	 */
-	spin_lock_irqsave(&idr_lock, *flags);
+
+	rcu_read_lock();
 	timr = idr_find(&posix_timers_id, (int)timer_id);
 	if (timr) {
-		spin_lock(&timr->it_lock);
-		if (timr->it_signal == current->signal) {
-			spin_unlock(&idr_lock);
+		spin_lock_irqsave(&timr->it_lock, *flags);
+		if (timr->it_id == (int)timer_id && timr->it_signal == current->signal) {
+			rcu_read_unlock();
 			return timr;
 		}
-		spin_unlock(&timr->it_lock);
+		spin_unlock_irqrestore(&timr->it_lock, *flags);
 	}
-	spin_unlock_irqrestore(&idr_lock, *flags);
+	rcu_read_unlock();
 
 	return NULL;
 }

[PATCH] posix-timers: RCU conversion

[Eric Dumazet]

Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
a single spinlock (idr_lock) in posix-timers code, on its 48 core
machine.

Even on a 16 cpu machine (2x4x2), a single test can show 98% of cpu time
used in ticket_spin_lock, from lock_timer

Ref: http://www.spinics.net/lists/kvm/msg51526.html

Switching to RCU is quite easy, IDR being already RCU ready.

idr_lock should be locked only for an insert/delete, not a lookup.

Benchmark on a 2x4x2 machine, 16 processes calling timer_gettime().

Before :

real	1m18.669s
user	0m1.346s
sys	1m17.180s

After :

real	0m3.296s
user	0m1.366s
sys	0m1.926s


Reported-by: Ben Nagy <ben@iagu.net>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Avi Kivity <avi@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Richard Cochran <richard.cochran@omicron.at>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/posix-timers.h |    1 +
 kernel/posix-timers.c        |   25 ++++++++++++++-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index d51243a..5dc27ca 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -81,6 +81,7 @@ struct k_itimer {
 			unsigned long expires;
 		} mmtimer;
 	} it;
+	struct rcu_head rcu;
 };
 
 struct k_clock {
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 4c01249..acb9be9 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -491,6 +491,13 @@ static struct k_itimer * alloc_posix_timer(void)
 	return tmr;
 }
 
+static void k_itimer_rcu_free(struct rcu_head *head)
+{
+	struct k_itimer *tmr = container_of(head, struct k_itimer, rcu);
+
+	kmem_cache_free(posix_timers_cache, tmr);
+}
+
 #define IT_ID_SET	1
 #define IT_ID_NOT_SET	0
 static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
@@ -503,7 +510,7 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 	}
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
-	kmem_cache_free(posix_timers_cache, tmr);
+	call_rcu(&tmr->rcu, k_itimer_rcu_free);
 }
 
 static struct k_clock *clockid_to_kclock(const clockid_t id)
@@ -631,22 +638,18 @@ out:
 static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
 {
 	struct k_itimer *timr;
-	/*
-	 * Watch out here.  We do a irqsave on the idr_lock and pass the
-	 * flags part over to the timer lock.  Must not let interrupts in
-	 * while we are moving the lock.
-	 */
-	spin_lock_irqsave(&idr_lock, *flags);
+
+	rcu_read_lock();
 	timr = idr_find(&posix_timers_id, (int)timer_id);
 	if (timr) {
-		spin_lock(&timr->it_lock);
+		spin_lock_irqsave(&timr->it_lock, *flags);
 		if (timr->it_signal == current->signal) {
-			spin_unlock(&idr_lock);
+			rcu_read_unlock();
 			return timr;
 		}
-		spin_unlock(&timr->it_lock);
+		spin_unlock_irqrestore(&timr->it_lock, *flags);
 	}
-	spin_unlock_irqrestore(&idr_lock, *flags);
+	rcu_read_unlock();
 
 	return NULL;
 }

Re: [PATCH] posix-timers: RCU conversion

[Ben Nagy]

On Tue, Mar 22, 2011 at 12:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
> a single spinlock (idr_lock) in posix-timers code, on its 48 core
> machine.

Hi all,

Thanks a lot for all the help so far. We've tested with Eric's patch.

First up, here's our version of the patch for the current ubuntu
kernel from git:
http://paste.ubuntu.com/583668/

Here's top with 96 idle guests running:
op - 16:47:53 up  1:09,  3 users,  load average: 0.00, 0.01, 0.05
Tasks: 499 total,   3 running, 496 sleeping,   0 stopped,   0 zombie
Cpu(s):  1.9%us,  3.2%sy,  0.0%ni, 95.0%id,  0.0%wa,  0.0%hi,  0.0%si,  0.0%st
Mem:  99068656k total, 13121096k used, 85947560k free,    22192k buffers
Swap:  2438140k total,        0k used,  2438140k free,  3597860k cached
 (much better!)

Start of perf top:

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
   PerfTop:   10318 irqs/sec  kernel:97.4%  exact:  0.0% [1000Hz
cycles],  (all, 48 CPUs)
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

             samples  pcnt function                    DSO
             _______ _____ ___________________________
___________________________________________________________

            95444.00 59.3% __ticket_spin_lock
[kernel.kallsyms]
            12937.00  8.0% native_safe_halt
[kernel.kallsyms]
             6149.00  3.8% kvm_get_cs_db_l_bits
/lib/modules/2.6.38-7-server/kernel/arch/x86/kvm/kvm.ko
             5105.00  3.2% tg_load_down
[kernel.kallsyms]
             5088.00  3.2% svm_vcpu_run
/lib/modules/2.6.38-7-server/kernel/arch/x86/kvm/kvm-amd.ko
             4807.00  3.0% kvm_set_pfn_dirty
/lib/modules/2.6.38-7-server/kernel/arch/x86/kvm/kvm.ko
             2855.00  1.8% ktime_get
[kernel.kallsyms]
             1535.00  1.0% find_busiest_group
[kernel.kallsyms]
             1386.00  0.9% find_next_bit
[kernel.kallsyms]


Start of perf report -g
    55.26%            kvm  [kernel.kallsyms]     [k] __ticket_spin_lock
                      |
                      --- __ticket_spin_lock
                         |
                         |--94.68%-- _raw_spin_lock
                         |          |
                         |          |--97.55%-- double_rq_lock
                         |          |          load_balance
                         |          |          idle_balance
                         |          |          schedule
                         |          |          |
                         |          |          |--60.56%--
schedule_hrtimeout_range_clock
                         |          |          |
schedule_hrtimeout_range
                         |          |          |          poll_schedule_timeout
                         |          |          |          do_select
                         |          |          |          core_sys_select
                         |          |          |          sys_select
                         |          |          |          system_call_fastpath


Here is the perf.data from the unpatched (non debug) kernel
http://www.coseinc.com/woigbfwr32/perf.data

Here is the perf.data from the patched (non debug) kernel
http://www.coseinc.com/woigbfwr32/perf_patched.data

I think we're certainly in 'it's going to be useable' territory now,
but any further improvements or patches to test would of course be
gratefully received! Next step from my end is to test the guests under
load, unless there are any other suggestions.

I'm extremely impressed by the speed and professionalism of the
response to this problem, both from those on #kvm and the widening
circle of those on this email thread.

Many thanks!

Cheers,

ben

Re: [PATCH] posix-timers: RCU conversion

[Avi Kivity]

On 03/22/2011 10:59 AM, Ben Nagy wrote:
> On Tue, Mar 22, 2011 at 12:54 PM, Eric Dumazet<eric.dumazet@gmail.com>  wrote:
> >  Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
> >  a single spinlock (idr_lock) in posix-timers code, on its 48 core
> >  machine.
>
> Hi all,
>
> Thanks a lot for all the help so far. We've tested with Eric's patch.
>
> First up, here's our version of the patch for the current ubuntu
> kernel from git:
> http://paste.ubuntu.com/583668/
>
> Here's top with 96 idle guests running:
> op - 16:47:53 up  1:09,  3 users,  load average: 0.00, 0.01, 0.05
> Tasks: 499 total,   3 running, 496 sleeping,   0 stopped,   0 zombie
> Cpu(s):  1.9%us,  3.2%sy,  0.0%ni, 95.0%id,  0.0%wa,  0.0%hi,  0.0%si,  0.0%st
> Mem:  99068656k total, 13121096k used, 85947560k free,    22192k buffers
> Swap:  2438140k total,        0k used,  2438140k free,  3597860k cached
>   (much better!)
>
> Start of perf top:
>
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>     PerfTop:   10318 irqs/sec  kernel:97.4%  exact:  0.0% [1000Hz
> cycles],  (all, 48 CPUs)
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
>               samples  pcnt function                    DSO
>               _______ _____ ___________________________
> ___________________________________________________________
>
>              95444.00 59.3% __ticket_spin_lock
> [kernel.kallsyms]
>              12937.00  8.0% native_safe_halt
> [kernel.kallsyms]
>               6149.00  3.8% kvm_get_cs_db_l_bits
> /lib/modules/2.6.38-7-server/kernel/arch/x86/kvm/kvm.ko
>               5105.00  3.2% tg_load_down
> [kernel.kallsyms]
>               5088.00  3.2% svm_vcpu_run
> /lib/modules/2.6.38-7-server/kernel/arch/x86/kvm/kvm-amd.ko
>               4807.00  3.0% kvm_set_pfn_dirty
> /lib/modules/2.6.38-7-server/kernel/arch/x86/kvm/kvm.ko
>               2855.00  1.8% ktime_get
> [kernel.kallsyms]
>               1535.00  1.0% find_busiest_group
> [kernel.kallsyms]
>               1386.00  0.9% find_next_bit
> [kernel.kallsyms]
>
>
> Start of perf report -g
>      55.26%            kvm  [kernel.kallsyms]     [k] __ticket_spin_lock
>                        |
>                        --- __ticket_spin_lock
>                           |
>                           |--94.68%-- _raw_spin_lock
>                           |          |
>                           |          |--97.55%-- double_rq_lock
>                           |          |          load_balance
>                           |          |          idle_balance
>                           |          |          schedule
>                           |          |          |
>                           |          |          |--60.56%--
> schedule_hrtimeout_range_clock
>                           |          |          |
> schedule_hrtimeout_range
>                           |          |          |          poll_schedule_timeout
>                           |          |          |          do_select
>                           |          |          |          core_sys_select
>                           |          |          |          sys_select
>                           |          |          |          system_call_fastpath
>
>

Looks like the posix-timer issue is completely gone, to be replaced by 
the load balancer.

Copying peterz.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] posix-timers: RCU conversion

[Ben Nagy]

On Tue, Mar 22, 2011 at 4:20 PM, Avi Kivity <avi@redhat.com> wrote:
[...]
> Looks like the posix-timer issue is completely gone, to be replaced by the
> load balancer.
>
> Copying peterz.

Hi all,

I feel bad about such a big cc list, but I don't know who can be left out :/

Still got the performance issue with the load balancer.

Additionally, when trying to test with 48 VMs under load, they
consistently all lock up after a few minutes, which we traced as far
as ocfs2 dying. We're investigating it as an ocfs2 issue, and we'll
try and replicate on ext4 / nfs, but I just thought it might be
somehow related.

Apr  3 18:27:35 eax kernel: [16029.399507] ------------[ cut here ]------------
Apr  3 18:27:35 eax kernel: [16029.401408] kernel BUG at
/home/fuzzadmin/src/natty/source/fs/jbd2/journal.c:1610!
Apr  3 18:27:35 eax kernel: [16029.404541] invalid opcode: 0000 [#1] SMP
Apr  3 18:27:35 eax kernel: [16029.406289] last sysfs file:
/sys/devices/system/cpu/cpu47/cache/index2/shared_cpu_map
Apr  3 18:27:35 eax kernel: [16029.409453] CPU 36
Apr  3 18:27:35 eax kernel: [16029.409453] Modules linked in: ocfs2
quota_tree ip6table_filter ip6_tables ipt_MASQUERADE iptable_nat
nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntr
ack ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter
ip_tables x_tables bridge stp w83627ehf hwmon_vid ocfs2_dlmfs
ocfs2_stack_o2cb ocfs2_dlm ocfs2_nodemanager ocfs2_stackglue c
onfigfs ipmi_si ipmi_msghandler ib_srp scsi_transport_srp scsi_tgt
vesafb ib_ipoib ib_iser ib_umad iscsi_tcp rdma_ucm libiscsi_tcp
rdma_cm libiscsi ib_cm iw_cm scsi_transport_iscsi ib_addr ib
_sa ib_uverbs mlx4_ib ib_mad ib_core vhost_net amd64_edac_mod psmouse
edac_core sp5100_tco ghes joydev serio_raw hed edac_mce_amd k10temp
kvm_amd i2c_piix4 kvm lp parport usb_storage usbhid h
id uas igb ahci pata_atiixp libahci dca mlx4_core
Apr  3 18:27:35 eax kernel: [16029.409453]
Apr  3 18:27:35 eax kernel: [16029.409453] Pid: 1746, comm: ocfs2cmt
Not tainted 2.6.38-7-server #39 Supermicro H8QG6/H8QG6
Apr  3 18:27:35 eax kernel: [16029.409453] RIP:
0010:[<ffffffff8124905a>]  [<ffffffff8124905a>]
jbd2_journal_flush+0x17a/0x190
Apr  3 18:27:35 eax kernel: [16029.409453] RSP: 0018:ffff880404be3dc0
EFLAGS: 00010286
Apr  3 18:27:35 eax kernel: [16029.409453] RAX: 0000000000000029 RBX:
ffff88181593c000 RCX: 000000000000001e
Apr  3 18:27:35 eax kernel: [16029.409453] RDX: 00000000fffffffb RSI:
ffff880404be3cd0 RDI: ffff88181593c024
Apr  3 18:27:35 eax kernel: [16029.409453] RBP: ffff880404be3df0 R08:
ffff880404be2000 R09: 0000000000000000
Apr  3 18:27:35 eax kernel: [16029.409453] R10: 0000000000000000 R11:
0000000000000001 R12: 0000000000000e5d
Apr  3 18:27:35 eax kernel: [16029.409453] R13: ffff88181593c39c R14:
ffff88181593c024 R15: 0000000000000000
Apr  3 18:27:35 eax kernel: [16029.409453] FS:  00007f73affb7700(0000)
GS:ffff881627c00000(0000) knlGS:0000000000000000
Apr  3 18:27:35 eax kernel: [16029.736049] CS:  0010 DS: 0000 ES: 0000
CR0: 000000008005003b
Apr  3 18:27:35 eax kernel: [16029.736049] CR2: 0000000002f72000 CR3:
00000002f0d31000 CR4: 00000000000006e0
Apr  3 18:27:35 eax kernel: [16029.736049] DR0: 00000000000000a0 DR1:
0000000000000000 DR2: 0000000000000003
Apr  3 18:27:35 eax kernel: [16029.736049] DR3: 00000000000000b0 DR6:
00000000ffff0ff0 DR7: 0000000000000400
Apr  3 18:27:35 eax kernel: [16029.736049] Process ocfs2cmt (pid:
1746, threadinfo ffff880404be2000, task ffff8803f72396e0)
Apr  3 18:27:35 eax kernel: [16029.736049] Stack:
Apr  3 18:27:35 eax kernel: [16029.736049]  0000000000000001
ffff8806155eb800 ffff8806155eb838 ffff8803f72396e0
Apr  3 18:27:35 eax kernel: [16029.736049]  ffff8806160cb000
ffff8806160cb160 ffff880404be3e40 ffffffffa042af12
Apr  3 18:27:35 eax kernel: [16029.736049]  0000000000000000
ffff8806155eb860 0000000000000286 ffff8806155eb828
Apr  3 18:27:35 eax kernel: [16029.736049] Call Trace:
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffffa042af12>]
ocfs2_commit_cache+0xc2/0x330 [ocfs2]
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffffa042b1e1>]
ocfs2_commit_thread+0x61/0x210 [ocfs2]
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff81087870>] ?
autoremove_wake_function+0x0/0x40
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffffa042b180>] ?
ocfs2_commit_thread+0x0/0x210 [ocfs2]
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff81087126>]
kthread+0x96/0xa0
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff8100cde4>]
kernel_thread_helper+0x4/0x10
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff81087090>] ?
kthread+0x0/0xa0
Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff8100cde0>] ?
kernel_thread_helper+0x0/0x10
Apr  3 18:27:35 eax kernel: [16029.736049] Code: c0 5b 41 5c 41 5d 41
5e 41 5f c9 c3 0f 1f 44 00 00 4c 8b 63 58 4d 85 e4 0f 85 d2 fe ff ff
f0 81 43 24 00 00 00 01 e9 da fe ff ff <0f> 0b 0f 0b 0f 0b 0f 0b 0f 0b
66 66 66 2e 0f 1f 84 00 00 00 00
Apr  3 18:27:35 eax kernel: [16029.736049] RIP  [<ffffffff8124905a>]
jbd2_journal_flush+0x17a/0x190
Apr  3 18:27:35 eax kernel: [16031.320082] ------------[ cut here ]------------

Thanks for any feedback...

Cheers,

ben

Re: [PATCH] posix-timers: RCU conversion

[Avi Kivity]

On 04/04/2011 06:30 AM, Ben Nagy wrote:
> On Tue, Mar 22, 2011 at 4:20 PM, Avi Kivity<avi@redhat.com>  wrote:
> [...]
> >  Looks like the posix-timer issue is completely gone, to be replaced by the
> >  load balancer.
> >
> >  Copying peterz.
>
> Hi all,
>
> I feel bad about such a big cc list, but I don't know who can be left out :/
>
> Still got the performance issue with the load balancer.
>
> Additionally, when trying to test with 48 VMs under load, they
> consistently all lock up after a few minutes, which we traced as far
> as ocfs2 dying. We're investigating it as an ocfs2 issue, and we'll
> try and replicate on ext4 / nfs, but I just thought it might be
> somehow related.
>
> Apr  3 18:27:35 eax kernel: [16029.399507] ------------[ cut here ]------------
> Apr  3 18:27:35 eax kernel: [16029.401408] kernel BUG at
> /home/fuzzadmin/src/natty/source/fs/jbd2/journal.c:1610!
> Apr  3 18:27:35 eax kernel: [16029.404541] invalid opcode: 0000 [#1] SMP
> Apr  3 18:27:35 eax kernel: [16029.406289] last sysfs file:
> /sys/devices/system/cpu/cpu47/cache/index2/shared_cpu_map
> Apr  3 18:27:35 eax kernel: [16029.409453] CPU 36
> Apr  3 18:27:35 eax kernel: [16029.409453] Modules linked in: ocfs2
> quota_tree ip6table_filter ip6_tables ipt_MASQUERADE iptable_nat
> nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntr
> ack ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter
> ip_tables x_tables bridge stp w83627ehf hwmon_vid ocfs2_dlmfs
> ocfs2_stack_o2cb ocfs2_dlm ocfs2_nodemanager ocfs2_stackglue c
> onfigfs ipmi_si ipmi_msghandler ib_srp scsi_transport_srp scsi_tgt
> vesafb ib_ipoib ib_iser ib_umad iscsi_tcp rdma_ucm libiscsi_tcp
> rdma_cm libiscsi ib_cm iw_cm scsi_transport_iscsi ib_addr ib
> _sa ib_uverbs mlx4_ib ib_mad ib_core vhost_net amd64_edac_mod psmouse
> edac_core sp5100_tco ghes joydev serio_raw hed edac_mce_amd k10temp
> kvm_amd i2c_piix4 kvm lp parport usb_storage usbhid h
> id uas igb ahci pata_atiixp libahci dca mlx4_core
> Apr  3 18:27:35 eax kernel: [16029.409453]
> Apr  3 18:27:35 eax kernel: [16029.409453] Pid: 1746, comm: ocfs2cmt
> Not tainted 2.6.38-7-server #39 Supermicro H8QG6/H8QG6
> Apr  3 18:27:35 eax kernel: [16029.409453] RIP:
> 0010:[<ffffffff8124905a>]  [<ffffffff8124905a>]
> jbd2_journal_flush+0x17a/0x190
> Apr  3 18:27:35 eax kernel: [16029.409453] RSP: 0018:ffff880404be3dc0
> EFLAGS: 00010286
> Apr  3 18:27:35 eax kernel: [16029.409453] RAX: 0000000000000029 RBX:
> ffff88181593c000 RCX: 000000000000001e
> Apr  3 18:27:35 eax kernel: [16029.409453] RDX: 00000000fffffffb RSI:
> ffff880404be3cd0 RDI: ffff88181593c024
> Apr  3 18:27:35 eax kernel: [16029.409453] RBP: ffff880404be3df0 R08:
> ffff880404be2000 R09: 0000000000000000
> Apr  3 18:27:35 eax kernel: [16029.409453] R10: 0000000000000000 R11:
> 0000000000000001 R12: 0000000000000e5d
> Apr  3 18:27:35 eax kernel: [16029.409453] R13: ffff88181593c39c R14:
> ffff88181593c024 R15: 0000000000000000
> Apr  3 18:27:35 eax kernel: [16029.409453] FS:  00007f73affb7700(0000)
> GS:ffff881627c00000(0000) knlGS:0000000000000000
> Apr  3 18:27:35 eax kernel: [16029.736049] CS:  0010 DS: 0000 ES: 0000
> CR0: 000000008005003b
> Apr  3 18:27:35 eax kernel: [16029.736049] CR2: 0000000002f72000 CR3:
> 00000002f0d31000 CR4: 00000000000006e0
> Apr  3 18:27:35 eax kernel: [16029.736049] DR0: 00000000000000a0 DR1:
> 0000000000000000 DR2: 0000000000000003
> Apr  3 18:27:35 eax kernel: [16029.736049] DR3: 00000000000000b0 DR6:
> 00000000ffff0ff0 DR7: 0000000000000400
> Apr  3 18:27:35 eax kernel: [16029.736049] Process ocfs2cmt (pid:
> 1746, threadinfo ffff880404be2000, task ffff8803f72396e0)
> Apr  3 18:27:35 eax kernel: [16029.736049] Stack:
> Apr  3 18:27:35 eax kernel: [16029.736049]  0000000000000001
> ffff8806155eb800 ffff8806155eb838 ffff8803f72396e0
> Apr  3 18:27:35 eax kernel: [16029.736049]  ffff8806160cb000
> ffff8806160cb160 ffff880404be3e40 ffffffffa042af12
> Apr  3 18:27:35 eax kernel: [16029.736049]  0000000000000000
> ffff8806155eb860 0000000000000286 ffff8806155eb828
> Apr  3 18:27:35 eax kernel: [16029.736049] Call Trace:
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffffa042af12>]
> ocfs2_commit_cache+0xc2/0x330 [ocfs2]
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffffa042b1e1>]
> ocfs2_commit_thread+0x61/0x210 [ocfs2]
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff81087870>] ?
> autoremove_wake_function+0x0/0x40
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffffa042b180>] ?
> ocfs2_commit_thread+0x0/0x210 [ocfs2]
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff81087126>]
> kthread+0x96/0xa0
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff8100cde4>]
> kernel_thread_helper+0x4/0x10
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff81087090>] ?
> kthread+0x0/0xa0
> Apr  3 18:27:35 eax kernel: [16029.736049]  [<ffffffff8100cde0>] ?
> kernel_thread_helper+0x0/0x10
> Apr  3 18:27:35 eax kernel: [16029.736049] Code: c0 5b 41 5c 41 5d 41
> 5e 41 5f c9 c3 0f 1f 44 00 00 4c 8b 63 58 4d 85 e4 0f 85 d2 fe ff ff
> f0 81 43 24 00 00 00 01 e9 da fe ff ff<0f>  0b 0f 0b 0f 0b 0f 0b 0f 0b
> 66 66 66 2e 0f 1f 84 00 00 00 00
> Apr  3 18:27:35 eax kernel: [16029.736049] RIP  [<ffffffff8124905a>]
> jbd2_journal_flush+0x17a/0x190
> Apr  3 18:27:35 eax kernel: [16031.320082] ------------[ cut here ]------------

Please report this to the ocfs2 developers.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] posix-timers: RCU conversion

[Peter Zijlstra]

On Tue, 2011-03-22 at 12:35 +0200, Avi Kivity wrote:
> Looks like the posix-timer issue is completely gone, to be replaced by 
> the load balancer. 

-ENOINFO, no kernel version, no setup, no workload, no nothing.

Re: [PATCH] posix-timers: RCU conversion

[Avi Kivity]

On 04/05/2011 10:49 AM, Peter Zijlstra wrote:
> On Tue, 2011-03-22 at 12:35 +0200, Avi Kivity wrote:
> >  Looks like the posix-timer issue is completely gone, to be replaced by
> >  the load balancer.
>
> -ENOINFO, no kernel version, no setup, no workload, no nothing.

http://www.spinics.net/lists/kvm/msg51526.html

plus patch in

http://www.spinics.net/lists/kvm/msg51595.html

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] posix-timers: RCU conversion

[Peter Zijlstra]

On Tue, 2011-03-22 at 12:35 +0200, Avi Kivity wrote:
>> Here's top with 96 idle guests running:

On some hacked up 2.6.38 kernel...

> > Start of perf report -g
> >      55.26%            kvm  [kernel.kallsyms]     [k] __ticket_spin_lock
> >                        |
> >                        --- __ticket_spin_lock
> >                           |
> >                           |--94.68%-- _raw_spin_lock
> >                           |          |
> >                           |          |--97.55%-- double_rq_lock
> >                           |          |          load_balance
> >                           |          |          idle_balance
> >                           |          |          schedule
> >                           |          |          |
> >                           |          |          |--60.56%-- schedule_hrtimeout_range_clock
> >                           |          |          |          schedule_hrtimeout_range
> >                           |          |          |          poll_schedule_timeout
> >                           |          |          |          do_select
> >                           |          |          |          core_sys_select
> >                           |          |          |          sys_select
> >                           |          |          |          system_call_fastpath

Looks like your workload and idle balancing don't much like each-other.

What I think is happening is that all your 'idle' qemu thingies keep
waking up frequently and because you've got like twice the number of
qemu instances as you've got cpus there's a fair chance you'll have a
cpu with a pending task while another one goes idle.

(Why does qemu keep waking if its idle? broken NOHZ?)

So idle balancing is called when the cpu goes idle (context switch to
the idle thread) and tries to steal a pending task from another cpu,
clearly it keeps finding these tasks otherwise it wouldn't try to take
that lock.

Mike, you build in some idle balance throttle logic, but that seems
defeated here (possible because it keeps finding pending tasks to
migrate? still need morning juice).

Re: [PATCH] posix-timers: RCU conversion

[Avi Kivity]

On 04/05/2011 11:48 AM, Peter Zijlstra wrote:
> What I think is happening is that all your 'idle' qemu thingies keep
> waking up frequently and because you've got like twice the number of
> qemu instances as you've got cpus there's a fair chance you'll have a
> cpu with a pending task while another one goes idle.

Those qemus have two threads each (one vcpu thread, one io thread).

> (Why does qemu keep waking if its idle? broken NOHZ?)

Could be waking up due to guest wakeups, or qemu internal wakeups 
(display refresh) or due to guest timer sources which are masked away in 
the guest (if that's the case we should optimize it away).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] posix-timers: RCU conversion

[Peter Zijlstra]

On Tue, 2011-04-05 at 11:56 +0300, Avi Kivity wrote:
> 
> Could be waking up due to guest wakeups, or qemu internal wakeups 
> (display refresh) or due to guest timer sources which are masked away in 
> the guest (if that's the case we should optimize it away).

Right, so I guess we're all clutching at straws here :-)

Ben how usable is that system when its in that state? Could you run a
function trace or a trace with all kvm and sched trace-events enabled?

Re: [PATCH] posix-timers: RCU conversion

[Avi Kivity]

On 04/05/2011 12:03 PM, Peter Zijlstra wrote:
> On Tue, 2011-04-05 at 11:56 +0300, Avi Kivity wrote:
> >
> >  Could be waking up due to guest wakeups, or qemu internal wakeups
> >  (display refresh) or due to guest timer sources which are masked away in
> >  the guest (if that's the case we should optimize it away).
>
> Right, so I guess we're all clutching at straws here :-)
>
> Ben how usable is that system when its in that state? Could you run a
> function trace or a trace with all kvm and sched trace-events enabled?

Ben, to do that, look up http://www.linux-kvm.org/page/Tracing; the 
command for combined sched/kvm tracing is

# trace-cmd record -b 20000 -e kvm -e sched



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] posix-timers: RCU conversion

[Ben Nagy]

On Tue, Apr 5, 2011 at 2:48 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Tue, 2011-04-05 at 11:56 +0300, Avi Kivity wrote:
>>
>> Could be waking up due to guest wakeups, or qemu internal wakeups
>> (display refresh) or due to guest timer sources which are masked away in
>> the guest (if that's the case we should optimize it away).
>
> Right, so I guess we're all clutching at straws here :-)
>
> Ben how usable is that system when its in that state? Could you run a
> function trace or a trace with all kvm and sched trace-events enabled?

I'm just rebuilding the storage on the network to work around an ocfs2
kernel oops (trying nfs/rdma) so I can't test anything just at the
moment.

I ran some tests under load with the local ext4 ssd, and, weirdly,
everything looked to be just fine - the huge bulk of the system time
was in svm_vcpu_run, which is as it should be I guess, but that was
with only 60 loaded guests.

I'll be able to repeat the same workload test tomorrow, and I'll see
how the perf top stuff looks. I should also be able to repeat the '96
idle guests' test and see if it's the same - if so we'll do that
tracing. My kernel's a moving target at the moment, sorry, we're
tracking the natty git (with Eric's rcu patch merged in).

Thanks, all,

ben

Re: [PATCH] posix-timers: RCU conversion

[Mike Galbraith]

On Tue, 2011-04-05 at 10:48 +0200, Peter Zijlstra wrote:
> On Tue, 2011-03-22 at 12:35 +0200, Avi Kivity wrote:
> >> Here's top with 96 idle guests running:
> 
> On some hacked up 2.6.38 kernel...
> 
> > > Start of perf report -g
> > >      55.26%            kvm  [kernel.kallsyms]     [k] __ticket_spin_lock
> > >                        |
> > >                        --- __ticket_spin_lock
> > >                           |
> > >                           |--94.68%-- _raw_spin_lock
> > >                           |          |
> > >                           |          |--97.55%-- double_rq_lock
> > >                           |          |          load_balance
> > >                           |          |          idle_balance
> > >                           |          |          schedule
> > >                           |          |          |
> > >                           |          |          |--60.56%-- schedule_hrtimeout_range_clock
> > >                           |          |          |          schedule_hrtimeout_range
> > >                           |          |          |          poll_schedule_timeout
> > >                           |          |          |          do_select
> > >                           |          |          |          core_sys_select
> > >                           |          |          |          sys_select
> > >                           |          |          |          system_call_fastpath
> 
> Looks like your workload and idle balancing don't much like each-other.
> 
> What I think is happening is that all your 'idle' qemu thingies keep
> waking up frequently and because you've got like twice the number of
> qemu instances as you've got cpus there's a fair chance you'll have a
> cpu with a pending task while another one goes idle.
> 
> (Why does qemu keep waking if its idle? broken NOHZ?)
> 
> So idle balancing is called when the cpu goes idle (context switch to
> the idle thread) and tries to steal a pending task from another cpu,
> clearly it keeps finding these tasks otherwise it wouldn't try to take
> that lock.
> 
> Mike, you build in some idle balance throttle logic, but that seems
> defeated here (possible because it keeps finding pending tasks to
> migrate? still need morning juice).

Hm.  Maybe someone set sysctl_sched_migration_cost to 0, or wakeups
aren't arriving that frequently is all that pops to mind atm.

	-Mike

[PATCH] posix-timers: RCU conversion

[Eric Dumazet]

Andrew, I submitted this patch some time ago and got no feedback from
Thomas, John, Richard or Paul.

https://lkml.org/lkml/2011/3/22/29

Could you please take it in mm ?

Thanks

[PATCH] posix-timers: RCU conversion

Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
a single spinlock (idr_lock) in posix-timers code, on its 48 core
machine.

Even on a 16 cpu machine (2x4x2), a single test can show 98% of cpu time
used in ticket_spin_lock, from lock_timer

Ref: http://www.spinics.net/lists/kvm/msg51526.html

Switching to RCU is quite easy, IDR being already RCU ready.

idr_lock should be locked only for an insert/delete, not a lookup.

Benchmark on a 2x4x2 machine, 16 processes calling timer_gettime().

Before :

real    1m18.669s
user    0m1.346s
sys     1m17.180s

After :

real    0m3.296s
user    0m1.366s
sys     0m1.926s

Reported-by: Ben Nagy <ben@iagu.net>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Tested-by: Ben Nagy <ben@iagu.net>
Cc: Avi Kivity <avi@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Richard Cochran <richard.cochran@omicron.at>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/posix-timers.h |    1 +
 kernel/posix-timers.c        |   25 ++++++++++++++-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index d51243a..5dc27ca 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -81,6 +81,7 @@ struct k_itimer {
 			unsigned long expires;
 		} mmtimer;
 	} it;
+	struct rcu_head rcu;
 };
 
 struct k_clock {
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 4c01249..acb9be9 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -491,6 +491,13 @@ static struct k_itimer * alloc_posix_timer(void)
 	return tmr;
 }
 
+static void k_itimer_rcu_free(struct rcu_head *head)
+{
+	struct k_itimer *tmr = container_of(head, struct k_itimer, rcu);
+
+	kmem_cache_free(posix_timers_cache, tmr);
+}
+
 #define IT_ID_SET	1
 #define IT_ID_NOT_SET	0
 static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
@@ -503,7 +510,7 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 	}
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
-	kmem_cache_free(posix_timers_cache, tmr);
+	call_rcu(&tmr->rcu, k_itimer_rcu_free);
 }
 
 static struct k_clock *clockid_to_kclock(const clockid_t id)
@@ -631,22 +638,18 @@ out:
 static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
 {
 	struct k_itimer *timr;
-	/*
-	 * Watch out here.  We do a irqsave on the idr_lock and pass the
-	 * flags part over to the timer lock.  Must not let interrupts in
-	 * while we are moving the lock.
-	 */
-	spin_lock_irqsave(&idr_lock, *flags);
+
+	rcu_read_lock();
 	timr = idr_find(&posix_timers_id, (int)timer_id);
 	if (timr) {
-		spin_lock(&timr->it_lock);
+		spin_lock_irqsave(&timr->it_lock, *flags);
 		if (timr->it_signal == current->signal) {
-			spin_unlock(&idr_lock);
+			rcu_read_unlock();
 			return timr;
 		}
-		spin_unlock(&timr->it_lock);
+		spin_unlock_irqrestore(&timr->it_lock, *flags);
 	}
-	spin_unlock_irqrestore(&idr_lock, *flags);
+	rcu_read_unlock();
 
 	return NULL;
 }

Re: [PATCH] posix-timers: RCU conversion

[john stultz]

On Sun, 2011-04-03 at 18:54 +0200, Eric Dumazet wrote:
> Andrew, I submitted this patch some time ago and got no feedback from
> Thomas, John, Richard or Paul.

Sorry about that. Its been on my list.

It looks ok to me, but I not very familiar with idr locking rules
(though looking at the code the comments state that idr_find can be
safely called the way you do here).

CC'ing Oleg as I think he knows the code and is always good at catching
things.


thanks
-john


> [PATCH] posix-timers: RCU conversion
> 
> Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
> a single spinlock (idr_lock) in posix-timers code, on its 48 core
> machine.
> 
> Even on a 16 cpu machine (2x4x2), a single test can show 98% of cpu time
> used in ticket_spin_lock, from lock_timer
> 
> Ref: http://www.spinics.net/lists/kvm/msg51526.html
> 
> Switching to RCU is quite easy, IDR being already RCU ready.
> 
> idr_lock should be locked only for an insert/delete, not a lookup.
> 
> Benchmark on a 2x4x2 machine, 16 processes calling timer_gettime().
> 
> Before :
> 
> real    1m18.669s
> user    0m1.346s
> sys     1m17.180s
> 
> After :
> 
> real    0m3.296s
> user    0m1.366s
> sys     0m1.926s
> 
> Reported-by: Ben Nagy <ben@iagu.net>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Tested-by: Ben Nagy <ben@iagu.net>
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Richard Cochran <richard.cochran@omicron.at>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  include/linux/posix-timers.h |    1 +
>  kernel/posix-timers.c        |   25 ++++++++++++++-----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
> index d51243a..5dc27ca 100644
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
> @@ -81,6 +81,7 @@ struct k_itimer {
>  			unsigned long expires;
>  		} mmtimer;
>  	} it;
> +	struct rcu_head rcu;
>  };
> 
>  struct k_clock {
> diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
> index 4c01249..acb9be9 100644
> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -491,6 +491,13 @@ static struct k_itimer * alloc_posix_timer(void)
>  	return tmr;
>  }
> 
> +static void k_itimer_rcu_free(struct rcu_head *head)
> +{
> +	struct k_itimer *tmr = container_of(head, struct k_itimer, rcu);
> +
> +	kmem_cache_free(posix_timers_cache, tmr);
> +}
> +
>  #define IT_ID_SET	1
>  #define IT_ID_NOT_SET	0
>  static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
> @@ -503,7 +510,7 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
>  	}
>  	put_pid(tmr->it_pid);
>  	sigqueue_free(tmr->sigq);
> -	kmem_cache_free(posix_timers_cache, tmr);
> +	call_rcu(&tmr->rcu, k_itimer_rcu_free);
>  }
> 
>  static struct k_clock *clockid_to_kclock(const clockid_t id)
> @@ -631,22 +638,18 @@ out:
>  static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
>  {
>  	struct k_itimer *timr;
> -	/*
> -	 * Watch out here.  We do a irqsave on the idr_lock and pass the
> -	 * flags part over to the timer lock.  Must not let interrupts in
> -	 * while we are moving the lock.
> -	 */
> -	spin_lock_irqsave(&idr_lock, *flags);
> +
> +	rcu_read_lock();
>  	timr = idr_find(&posix_timers_id, (int)timer_id);
>  	if (timr) {
> -		spin_lock(&timr->it_lock);
> +		spin_lock_irqsave(&timr->it_lock, *flags);
>  		if (timr->it_signal == current->signal) {
> -			spin_unlock(&idr_lock);
> +			rcu_read_unlock();
>  			return timr;
>  		}
> -		spin_unlock(&timr->it_lock);
> +		spin_unlock_irqrestore(&timr->it_lock, *flags);
>  	}
> -	spin_unlock_irqrestore(&idr_lock, *flags);
> +	rcu_read_unlock();
> 
>  	return NULL;
>  }
> 
> 

Re: [PATCH] posix-timers: RCU conversion

[Eric Dumazet]

Le lundi 04 avril 2011 à 11:08 -0700, john stultz a écrit :
> On Sun, 2011-04-03 at 18:54 +0200, Eric Dumazet wrote:
> > Andrew, I submitted this patch some time ago and got no feedback from
> > Thomas, John, Richard or Paul.
> 
> Sorry about that. Its been on my list.
> 
> It looks ok to me, but I not very familiar with idr locking rules
> (though looking at the code the comments state that idr_find can be
> safely called the way you do here).

Not only comments, but ipc uses rcu locking as well ;)

Nadia Derbey did the work on idr and ipc in commits :

983bfb7db303cfde56ae5bbf4e0f2f46e38c9576
(ipc: call idr_find() without locking in ipc_lock())

f9c46d6ea5ce138a886c3a0f10a46130afab75f5
(idr: make idr_find rcu-safe)

Re: [PATCH] posix-timers: RCU conversion

[Oleg Nesterov]

On 04/04, john stultz wrote:
>
> On Sun, 2011-04-03 at 18:54 +0200, Eric Dumazet wrote:
> > --- a/include/linux/posix-timers.h
> > +++ b/include/linux/posix-timers.h
> > @@ -81,6 +81,7 @@ struct k_itimer {
> >  			unsigned long expires;
> >  		} mmtimer;
> >  	} it;
> > +	struct rcu_head rcu;
> >  };

Can't we move this rcu_head into the union above?

> >  struct k_clock {
> > diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
> > index 4c01249..acb9be9 100644
> > --- a/kernel/posix-timers.c
> > +++ b/kernel/posix-timers.c
> > @@ -491,6 +491,13 @@ static struct k_itimer * alloc_posix_timer(void)
> >  	return tmr;
> >  }
> >
> > +static void k_itimer_rcu_free(struct rcu_head *head)
> > +{
> > +	struct k_itimer *tmr = container_of(head, struct k_itimer, rcu);
> > +
> > +	kmem_cache_free(posix_timers_cache, tmr);
> > +}

Not that I really think it makes sense to change the patch... but we
could even use SLAB_DESTROY_BY_RCU, this is more effective. All we
need is ctor which sets ->it_signal = NULL and initializes ->it_lock
for lock_timer().

> >  static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
> >  {
> >  	struct k_itimer *timr;
> > -	/*
> > -	 * Watch out here.  We do a irqsave on the idr_lock and pass the
> > -	 * flags part over to the timer lock.  Must not let interrupts in
> > -	 * while we are moving the lock.
> > -	 */
> > -	spin_lock_irqsave(&idr_lock, *flags);
> > +
> > +	rcu_read_lock();
> >  	timr = idr_find(&posix_timers_id, (int)timer_id);
> >  	if (timr) {
> > -		spin_lock(&timr->it_lock);
> > +		spin_lock_irqsave(&timr->it_lock, *flags);
> >  		if (timr->it_signal == current->signal) {
> > -			spin_unlock(&idr_lock);
> > +			rcu_read_unlock();
> >  			return timr;
> >  		}
> > -		spin_unlock(&timr->it_lock);
> > +		spin_unlock_irqrestore(&timr->it_lock, *flags);
> >  	}
> > -	spin_unlock_irqrestore(&idr_lock, *flags);
> > +	rcu_read_unlock();

I think this is correct.

The question is, why do we use the global database for the timer ids.
All timers live in signal_struct->posix_timers anyway, perhaps we could
use a per-process array instead.

Oleg.

[PATCH v2] posix-timers: RCU conversion

[Eric Dumazet]

Le mardi 05 avril 2011 à 16:48 +0200, Oleg Nesterov a écrit :

> Can't we move this rcu_head into the union above?
> 

Yes, you're right.


> 
> Not that I really think it makes sense to change the patch... but we
> could even use SLAB_DESTROY_BY_RCU, this is more effective. All we
> need is ctor which sets ->it_signal = NULL and initializes ->it_lock
> for lock_timer().
> 

I considered this, but this means SLUB cannot merge the
posix_timers_cache anymore.

This wastes more ram (each cpu has its own slab caches),
and considering posix-timers are seldom used...


> The question is, why do we use the global database for the timer ids.
> All timers live in signal_struct->posix_timers anyway, perhaps we could
> use a per-process array instead.
> 

This would add some overhead at process creation (to initialize the
'array' or whatever tree root).

It would help some workloads, (create/delete timers from lot of
different processes/cpus). I am not sure we really need this right now,
since we waited 2011 before even trying to optimize read side ;)

Thanks

[PATCH v2] posix-timers: RCU conversion

Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
a single spinlock (idr_lock) in posix-timers code, on its 48 core
machine.

Even on a 16 cpu machine (2x4x2), a single test can show 98% of cpu time
used in ticket_spin_lock, from lock_timer

Ref: http://www.spinics.net/lists/kvm/msg51526.html

Switching to RCU is quite easy, IDR being already RCU ready.

idr_lock should be locked only for an insert/delete, not a lookup.

Benchmark on a 2x4x2 machine, 16 processes calling timer_gettime().

Before :

real    1m18.669s
user    0m1.346s
sys     1m17.180s

After :

real    0m3.296s
user    0m1.366s
sys     0m1.926s

Reported-by: Ben Nagy <ben@iagu.net>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Tested-by: Ben Nagy <ben@iagu.net>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Avi Kivity <avi@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Richard Cochran <richard.cochran@omicron.at>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
V2 : rcu_head put in the 'it' union, as Oleg suggested.

 include/linux/posix-timers.h |    1 +
 kernel/posix-timers.c        |   25 ++++++++++++++-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index d51243a..ea0db16 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -80,6 +80,7 @@ struct k_itimer {
 			unsigned long incr;
 			unsigned long expires;
 		} mmtimer;
+		struct rcu_head rcu;
 	} it;
 };
 
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 4c01249..4a428b0 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -491,6 +491,13 @@ static struct k_itimer * alloc_posix_timer(void)
 	return tmr;
 }
 
+static void k_itimer_rcu_free(struct rcu_head *head)
+{
+	struct k_itimer *tmr = container_of(head, struct k_itimer, it.rcu);
+
+	kmem_cache_free(posix_timers_cache, tmr);
+}
+
 #define IT_ID_SET	1
 #define IT_ID_NOT_SET	0
 static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
@@ -503,7 +510,7 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 	}
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
-	kmem_cache_free(posix_timers_cache, tmr);
+	call_rcu(&tmr->it.rcu, k_itimer_rcu_free);
 }
 
 static struct k_clock *clockid_to_kclock(const clockid_t id)
@@ -631,22 +638,18 @@ out:
 static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
 {
 	struct k_itimer *timr;
-	/*
-	 * Watch out here.  We do a irqsave on the idr_lock and pass the
-	 * flags part over to the timer lock.  Must not let interrupts in
-	 * while we are moving the lock.
-	 */
-	spin_lock_irqsave(&idr_lock, *flags);
+
+	rcu_read_lock();
 	timr = idr_find(&posix_timers_id, (int)timer_id);
 	if (timr) {
-		spin_lock(&timr->it_lock);
+		spin_lock_irqsave(&timr->it_lock, *flags);
 		if (timr->it_signal == current->signal) {
-			spin_unlock(&idr_lock);
+			rcu_read_unlock();
 			return timr;
 		}
-		spin_unlock(&timr->it_lock);
+		spin_unlock_irqrestore(&timr->it_lock, *flags);
 	}
-	spin_unlock_irqrestore(&idr_lock, *flags);
+	rcu_read_unlock();
 
 	return NULL;
 }

Re: [PATCH v2] posix-timers: RCU conversion

[Oleg Nesterov]

On 04/05, Eric Dumazet wrote:
>
> Le mardi 05 avril 2011 à 16:48 +0200, Oleg Nesterov a écrit :
> >
> > Not that I really think it makes sense to change the patch... but we
> > could even use SLAB_DESTROY_BY_RCU, this is more effective. All we
> > need is ctor which sets ->it_signal = NULL and initializes ->it_lock
> > for lock_timer().
> >
>
> I considered this, but this means SLUB cannot merge the
> posix_timers_cache anymore.

OK, I see.

> > The question is, why do we use the global database for the timer ids.
> > All timers live in signal_struct->posix_timers anyway, perhaps we could
> > use a per-process array instead.
> >
>
> This would add some overhead at process creation (to initialize the
> 'array' or whatever tree root).

I am not sure. This initialization should be as simple as
"p->timers_array = NULL".

But,

> It would help some workloads, (create/delete timers from lot of
> different processes/cpus). I am not sure we really need this right now,
> since we waited 2011 before even trying to optimize read side ;)

Yes, agreed.

So, I think the patch is fine.

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH v2] posix-timers: RCU conversion

[Avi Kivity]

On 04/05/2011 06:18 PM, Eric Dumazet wrote:
> [PATCH v2] posix-timers: RCU conversion
>

<snip>

Andrew, I see this in -mmotm, but I see v1, not v2.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] posix-timers: RCU conversion

[Eric Dumazet]

Le lundi 16 mai 2011 à 18:10 +0300, Avi Kivity a écrit :
> On 04/05/2011 06:18 PM, Eric Dumazet wrote:
> > [PATCH v2] posix-timers: RCU conversion
> >
> 
> <snip>
> 
> Andrew, I see this in -mmotm, but I see v1, not v2.
> 

Good catch, thanks

To be fair, v1 should work well too ;)


V2 : rcu_head put in the 'it' union, as Oleg suggested.

Re: [PATCH] posix-timers: RCU conversion

[Paul E. McKenney]

On Sun, Apr 03, 2011 at 06:54:20PM +0200, Eric Dumazet wrote:
> Andrew, I submitted this patch some time ago and got no feedback from
> Thomas, John, Richard or Paul.
> 
> https://lkml.org/lkml/2011/3/22/29
> 
> Could you please take it in mm ?
> 
> Thanks
> 
> [PATCH] posix-timers: RCU conversion
> 
> Ben Nagy reported a scalability problem with KVM/QEMU that hit very hard
> a single spinlock (idr_lock) in posix-timers code, on its 48 core
> machine.
> 
> Even on a 16 cpu machine (2x4x2), a single test can show 98% of cpu time
> used in ticket_spin_lock, from lock_timer
> 
> Ref: http://www.spinics.net/lists/kvm/msg51526.html
> 
> Switching to RCU is quite easy, IDR being already RCU ready.
> 
> idr_lock should be locked only for an insert/delete, not a lookup.
> 
> Benchmark on a 2x4x2 machine, 16 processes calling timer_gettime().
> 
> Before :
> 
> real    1m18.669s
> user    0m1.346s
> sys     1m17.180s
> 
> After :
> 
> real    0m3.296s
> user    0m1.366s
> sys     0m1.926s

Good stuff!

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Reported-by: Ben Nagy <ben@iagu.net>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Tested-by: Ben Nagy <ben@iagu.net>
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Richard Cochran <richard.cochran@omicron.at>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  include/linux/posix-timers.h |    1 +
>  kernel/posix-timers.c        |   25 ++++++++++++++-----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
> index d51243a..5dc27ca 100644
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
> @@ -81,6 +81,7 @@ struct k_itimer {
>  			unsigned long expires;
>  		} mmtimer;
>  	} it;
> +	struct rcu_head rcu;
>  };
> 
>  struct k_clock {
> diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
> index 4c01249..acb9be9 100644
> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -491,6 +491,13 @@ static struct k_itimer * alloc_posix_timer(void)
>  	return tmr;
>  }
> 
> +static void k_itimer_rcu_free(struct rcu_head *head)
> +{
> +	struct k_itimer *tmr = container_of(head, struct k_itimer, rcu);
> +
> +	kmem_cache_free(posix_timers_cache, tmr);
> +}
> +
>  #define IT_ID_SET	1
>  #define IT_ID_NOT_SET	0
>  static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
> @@ -503,7 +510,7 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
>  	}
>  	put_pid(tmr->it_pid);
>  	sigqueue_free(tmr->sigq);
> -	kmem_cache_free(posix_timers_cache, tmr);
> +	call_rcu(&tmr->rcu, k_itimer_rcu_free);
>  }
> 
>  static struct k_clock *clockid_to_kclock(const clockid_t id)
> @@ -631,22 +638,18 @@ out:
>  static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
>  {
>  	struct k_itimer *timr;
> -	/*
> -	 * Watch out here.  We do a irqsave on the idr_lock and pass the
> -	 * flags part over to the timer lock.  Must not let interrupts in
> -	 * while we are moving the lock.
> -	 */
> -	spin_lock_irqsave(&idr_lock, *flags);
> +
> +	rcu_read_lock();
>  	timr = idr_find(&posix_timers_id, (int)timer_id);
>  	if (timr) {
> -		spin_lock(&timr->it_lock);
> +		spin_lock_irqsave(&timr->it_lock, *flags);
>  		if (timr->it_signal == current->signal) {
> -			spin_unlock(&idr_lock);
> +			rcu_read_unlock();
>  			return timr;
>  		}
> -		spin_unlock(&timr->it_lock);
> +		spin_unlock_irqrestore(&timr->it_lock, *flags);
>  	}
> -	spin_unlock_irqrestore(&idr_lock, *flags);
> +	rcu_read_unlock();
> 
>  	return NULL;
>  }
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/