Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL
> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@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)
> 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
Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL
On 6 February 2018 at 08:24, Gonglei (Arei) 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
Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL
> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@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 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
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 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
Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL
On 1 February 2018 at 14:23, Paolo Bonzini 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
Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL
> -Original Message- > From: Paolo Bonzini [mailto:pbonz...@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 callserrors syscall > > -- --- --- - - > > 93.870.119070 30 4000 ppoll > > 3.270.004148 2 2038 ioctl > > 2.660.003370 2 2014 futex > > 0.090.000113 1 106 read > > 0.090.000109 1 104 io_getevents > > 0.020.29 130 poll > > 0.000.00 0 1 write > > -- --- --- - - > > 100.000.126839 8293 total > > > > After appling the change: > > > > % time seconds usecs/call callserrors syscall > > -- --- --- - - > > 92.860.067441 16 4094 ppoll > > 4.850.003522 2 2136 ioctl > > 1.170.000850 4 189 futex > > 0.540.000395 2 202 read > > 0.520.000379 2 202 io_getevents > > 0.050.37 130 poll > > -- --- --- - - > > 100.000.072624 6853 total > > > > The futex call number decreases ~90.6% on an idle windows 7 guest. > > > > Signed-off-by: Gonglei > > --- > > 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
Re: [Qemu-devel] [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 callserrors syscall > -- --- --- - - > 93.870.119070 30 4000 ppoll > 3.270.004148 2 2038 ioctl > 2.660.003370 2 2014 futex > 0.090.000113 1 106 read > 0.090.000109 1 104 io_getevents > 0.020.29 130 poll > 0.000.00 0 1 write > -- --- --- - - > 100.000.126839 8293 total > > After appling the change: > > % time seconds usecs/call callserrors syscall > -- --- --- - - > 92.860.067441 16 4094 ppoll > 4.850.003522 2 2136 ioctl > 1.170.000850 4 189 futex > 0.540.000395 2 202 read > 0.520.000379 2 202 io_getevents > 0.050.37 130 poll > -- --- --- - - > 100.000.072624 6853 total > > The futex call number decreases ~90.6% on an idle windows 7 guest. > > Signed-off-by: Gonglei > --- > 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
