As windows guest use rtc as the clock source device,
and access rtc frequently. Let's move the rtc memory
region outside BQL to decrease overhead for windows guests.
strace -tt -p $1 -c -o result_$1.log &
sleep $2
pid=$(pidof strace)
kill $pid
cat result_$1.log
Before appling this change:
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
93.87 0.119070 30 4000 ppoll
3.27 0.004148 2 2038 ioctl
2.66 0.003370 2 2014 futex
0.09 0.000113 1 106 read
0.09 0.000109 1 104 io_getevents
0.02 0.000029 1 30 poll
0.00 0.000000 0 1 write
------ ----------- ----------- --------- --------- ----------------
100.00 0.126839 8293 total
After appling the change:
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
92.86 0.067441 16 4094 ppoll
4.85 0.003522 2 2136 ioctl
1.17 0.000850 4 189 futex
0.54 0.000395 2 202 read
0.52 0.000379 2 202 io_getevents
0.05 0.000037 1 30 poll
------ ----------- ----------- --------- --------- ----------------
100.00 0.072624 6853 total
The futex call number decreases ~90.6% on an idle windows 7 guest.
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
hw/timer/mc146818rtc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 35a05a6..d9d99c5 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -986,6 +986,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
qemu_register_suspend_notifier(&s->suspend_notifier);
memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2);
+ memory_region_clear_global_locking(&s->io);
isa_register_ioport(isadev, &s->io, base);
qdev_set_legacy_instance_id(dev, base, 3);
--
1.8.3.1
On 01/02/2018 08:47, Gonglei wrote: > As windows guest use rtc as the clock source device, > and access rtc frequently. Let's move the rtc memory > region outside BQL to decrease overhead for windows guests. > > strace -tt -p $1 -c -o result_$1.log & > sleep $2 > pid=$(pidof strace) > kill $pid > cat result_$1.log > > Before appling this change: > > % time seconds usecs/call calls errors syscall > ------ ----------- ----------- --------- --------- ---------------- > 93.87 0.119070 30 4000 ppoll > 3.27 0.004148 2 2038 ioctl > 2.66 0.003370 2 2014 futex > 0.09 0.000113 1 106 read > 0.09 0.000109 1 104 io_getevents > 0.02 0.000029 1 30 poll > 0.00 0.000000 0 1 write > ------ ----------- ----------- --------- --------- ---------------- > 100.00 0.126839 8293 total > > After appling the change: > > % time seconds usecs/call calls errors syscall > ------ ----------- ----------- --------- --------- ---------------- > 92.86 0.067441 16 4094 ppoll > 4.85 0.003522 2 2136 ioctl > 1.17 0.000850 4 189 futex > 0.54 0.000395 2 202 read > 0.52 0.000379 2 202 io_getevents > 0.05 0.000037 1 30 poll > ------ ----------- ----------- --------- --------- ---------------- > 100.00 0.072624 6853 total > > The futex call number decreases ~90.6% on an idle windows 7 guest. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/timer/mc146818rtc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c > index 35a05a6..d9d99c5 100644 > --- a/hw/timer/mc146818rtc.c > +++ b/hw/timer/mc146818rtc.c > @@ -986,6 +986,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp) > qemu_register_suspend_notifier(&s->suspend_notifier); > > memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2); > + memory_region_clear_global_locking(&s->io); > isa_register_ioport(isadev, &s->io, base); > > qdev_set_legacy_instance_id(dev, base, 3); > This is not enough, you need to add a new lock or something like that. Otherwise two vCPUs can access the RTC together and make a mess. Paolo
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Thursday, February 01, 2018 10:24 PM > To: Gonglei (Arei); qemu-devel@nongnu.org > Cc: Huangweidong (C) > Subject: Re: [PATCH] rtc: placing RTC memory region outside BQL > > On 01/02/2018 08:47, Gonglei wrote: > > As windows guest use rtc as the clock source device, > > and access rtc frequently. Let's move the rtc memory > > region outside BQL to decrease overhead for windows guests. > > > > strace -tt -p $1 -c -o result_$1.log & > > sleep $2 > > pid=$(pidof strace) > > kill $pid > > cat result_$1.log > > > > Before appling this change: > > > > % time seconds usecs/call calls errors syscall > > ------ ----------- ----------- --------- --------- ---------------- > > 93.87 0.119070 30 4000 ppoll > > 3.27 0.004148 2 2038 ioctl > > 2.66 0.003370 2 2014 futex > > 0.09 0.000113 1 106 read > > 0.09 0.000109 1 104 io_getevents > > 0.02 0.000029 1 30 poll > > 0.00 0.000000 0 1 write > > ------ ----------- ----------- --------- --------- ---------------- > > 100.00 0.126839 8293 total > > > > After appling the change: > > > > % time seconds usecs/call calls errors syscall > > ------ ----------- ----------- --------- --------- ---------------- > > 92.86 0.067441 16 4094 ppoll > > 4.85 0.003522 2 2136 ioctl > > 1.17 0.000850 4 189 futex > > 0.54 0.000395 2 202 read > > 0.52 0.000379 2 202 io_getevents > > 0.05 0.000037 1 30 poll > > ------ ----------- ----------- --------- --------- ---------------- > > 100.00 0.072624 6853 total > > > > The futex call number decreases ~90.6% on an idle windows 7 guest. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > hw/timer/mc146818rtc.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c > > index 35a05a6..d9d99c5 100644 > > --- a/hw/timer/mc146818rtc.c > > +++ b/hw/timer/mc146818rtc.c > > @@ -986,6 +986,7 @@ static void rtc_realizefn(DeviceState *dev, Error > **errp) > > qemu_register_suspend_notifier(&s->suspend_notifier); > > > > memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2); > > + memory_region_clear_global_locking(&s->io); > > isa_register_ioport(isadev, &s->io, base); > > > > qdev_set_legacy_instance_id(dev, base, 3); > > > > This is not enough, you need to add a new lock or something like that. > Otherwise two vCPUs can access the RTC together and make a mess. > Hi Paolo, Yes, that's true, although I have not encountered any problems yet. Let me enhance it in v2. Thanks, -Gonglei
On 1 February 2018 at 14:23, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 01/02/2018 08:47, Gonglei wrote: >> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c >> index 35a05a6..d9d99c5 100644 >> --- a/hw/timer/mc146818rtc.c >> +++ b/hw/timer/mc146818rtc.c >> @@ -986,6 +986,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp) >> qemu_register_suspend_notifier(&s->suspend_notifier); >> >> memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2); >> + memory_region_clear_global_locking(&s->io); >> isa_register_ioport(isadev, &s->io, base); >> >> qdev_set_legacy_instance_id(dev, base, 3); >> > > This is not enough, you need to add a new lock or something like that. > Otherwise two vCPUs can access the RTC together and make a mess. Do you also need to do something to take the global lock before raising the outbound IRQ line (since it might be connected to a device that does need the global lock), or am I confused ? thanks -- PMM
On 04/02/2018 19:02, Peter Maydell wrote: > On 1 February 2018 at 14:23, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 01/02/2018 08:47, Gonglei wrote: >>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c >>> index 35a05a6..d9d99c5 100644 >>> --- a/hw/timer/mc146818rtc.c >>> +++ b/hw/timer/mc146818rtc.c >>> @@ -986,6 +986,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp) >>> qemu_register_suspend_notifier(&s->suspend_notifier); >>> >>> memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2); >>> + memory_region_clear_global_locking(&s->io); >>> isa_register_ioport(isadev, &s->io, base); >>> >>> qdev_set_legacy_instance_id(dev, base, 3); >>> >> >> This is not enough, you need to add a new lock or something like that. >> Otherwise two vCPUs can access the RTC together and make a mess. > > Do you also need to do something to take the global lock before > raising the outbound IRQ line (since it might be connected to a device > that does need the global lock), or am I confused ? Yes, that's a good point. Most of the time the IRQ line is raised in a timer, but not always. Paolo
> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Monday, February 05, 2018 10:04 PM
> To: Peter Maydell
> Cc: Gonglei (Arei); QEMU Developers; Huangweidong (C)
> Subject: Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL
>
> On 04/02/2018 19:02, Peter Maydell wrote:
> > On 1 February 2018 at 14:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> On 01/02/2018 08:47, Gonglei wrote:
> >>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> >>> index 35a05a6..d9d99c5 100644
> >>> --- a/hw/timer/mc146818rtc.c
> >>> +++ b/hw/timer/mc146818rtc.c
> >>> @@ -986,6 +986,7 @@ static void rtc_realizefn(DeviceState *dev, Error
> **errp)
> >>> qemu_register_suspend_notifier(&s->suspend_notifier);
> >>>
> >>> memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2);
> >>> + memory_region_clear_global_locking(&s->io);
> >>> isa_register_ioport(isadev, &s->io, base);
> >>>
> >>> qdev_set_legacy_instance_id(dev, base, 3);
> >>>
> >>
> >> This is not enough, you need to add a new lock or something like that.
> >> Otherwise two vCPUs can access the RTC together and make a mess.
> >
> > Do you also need to do something to take the global lock before
> > raising the outbound IRQ line (since it might be connected to a device
> > that does need the global lock), or am I confused ?
>
> Yes, that's a good point. Most of the time the IRQ line is raised in a
> timer, but not always.
>
So, taking BQL is necessary, and what we can do is trying our best to narrow
down the process of locking ? For example, do the following wrapping:
static void rtc_rasie_irq(RTCState *s)
{
qemu_mutex_lock_iothread();
qemu_irq_raise(s->irq);
qemu_mutex_unlock_iothread();
}
static void rtc_lower_irq(RTCState *s)
{
qemu_mutex_lock_iothread();
qemu_irq_lower(s->irq);
qemu_mutex_unlock_iothread();
}
Thanks,
-Gonglei
On 6 February 2018 at 08:24, Gonglei (Arei) <arei.gonglei@huawei.com> wrote:
> So, taking BQL is necessary, and what we can do is trying our best to narrow
> down the process of locking ? For example, do the following wrapping:
>
> static void rtc_rasie_irq(RTCState *s)
> {
> qemu_mutex_lock_iothread();
> qemu_irq_raise(s->irq);
> qemu_mutex_unlock_iothread();
> }
>
> static void rtc_lower_irq(RTCState *s)
> {
> qemu_mutex_lock_iothread();
> qemu_irq_lower(s->irq);
> qemu_mutex_unlock_iothread();
> }
If you do that you'll also need to be careful about not calling
those functions from contexts where you already hold the iothread
mutex (eg timer callbacks), since you can't lock a mutex you
already have locked.
thanks
-- PMM
> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Tuesday, February 06, 2018 5:49 PM
> To: Gonglei (Arei)
> Cc: Paolo Bonzini; QEMU Developers; Huangweidong (C)
> Subject: Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL
>
> On 6 February 2018 at 08:24, Gonglei (Arei) <arei.gonglei@huawei.com>
> wrote:
> > So, taking BQL is necessary, and what we can do is trying our best to narrow
> > down the process of locking ? For example, do the following wrapping:
> >
> > static void rtc_rasie_irq(RTCState *s)
> > {
> > qemu_mutex_lock_iothread();
> > qemu_irq_raise(s->irq);
> > qemu_mutex_unlock_iothread();
> > }
> >
> > static void rtc_lower_irq(RTCState *s)
> > {
> > qemu_mutex_lock_iothread();
> > qemu_irq_lower(s->irq);
> > qemu_mutex_unlock_iothread();
> > }
>
> If you do that you'll also need to be careful about not calling
> those functions from contexts where you already hold the iothread
> mutex (eg timer callbacks), since you can't lock a mutex you
> already have locked.
>
Exactly, all contexts caused by the main process. :)
Three timers callbacks, calling rtc_reset().
Thanks,
-Gonglei
© 2016 - 2024 Red Hat, Inc.