diff mbox

Use SIGIO with caution

Message ID 1306705831-59385-1-git-send-email-andreas.faerber@web.de
State New
Headers show

Commit Message

Andreas Färber May 29, 2011, 9:50 p.m. UTC
BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
and doesn't need to be blocked.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 cpus.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

Comments

Anthony Liguori May 31, 2011, 1:47 p.m. UTC | #1
On 05/29/2011 04:50 PM, Andreas Färber wrote:
> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
> and doesn't need to be blocked.
>
> Signed-off-by: Andreas Färber<andreas.faerber@web.de>

Anything to do with signal masks is never a trivial patch BTW...

But I actually think explicit handling of SIGIO is unneeded.  I think 
this is a hold over from the pre-I/O thread days where we selectively 
set SIGIO on certain file descriptors to make sure that when an IO fd 
became readable, we received a signal to break out of the KVM emulation 
loop.

Can the folks on CC confirm/deny?

I can't see any use of SIGIO in the current source tree.

Regards,

Anthony Liguori

> ---
>   cpus.c |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 1fc34b7..42af291 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -395,7 +395,9 @@ static int qemu_signal_init(void)
>       pthread_sigmask(SIG_UNBLOCK,&set, NULL);
>
>       sigemptyset(&set);
> +#ifdef SIGIO
>       sigaddset(&set, SIGIO);
> +#endif
>       sigaddset(&set, SIGALRM);
>       sigaddset(&set, SIG_IPI);
>       sigaddset(&set, SIGBUS);
> @@ -408,7 +410,9 @@ static int qemu_signal_init(void)
>            * We need to process timer signals synchronously to avoid a race
>            * between exit_request check and KVM vcpu entry.
>            */
> +#ifdef SIGIO
>           sigaddset(&set, SIGIO);
> +#endif
>           sigaddset(&set, SIGALRM);
>       }
>   #endif
> @@ -449,12 +453,16 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
>   #else
>       sigemptyset(&set);
>       sigaddset(&set, SIG_IPI);
> +#ifdef SIGIO
>       sigaddset(&set, SIGIO);
> +#endif
>       sigaddset(&set, SIGALRM);
>       pthread_sigmask(SIG_BLOCK,&set, NULL);
>
>       pthread_sigmask(SIG_BLOCK, NULL,&set);
> +#ifdef SIGIO
>       sigdelset(&set, SIGIO);
> +#endif
>       sigdelset(&set, SIGALRM);
>   #endif
>       sigdelset(&set, SIG_IPI);
Jan Kiszka May 31, 2011, 2:06 p.m. UTC | #2
On 2011-05-31 15:47, Anthony Liguori wrote:
> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>> and doesn't need to be blocked.
>>
>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
> 
> Anything to do with signal masks is never a trivial patch BTW...
> 
> But I actually think explicit handling of SIGIO is unneeded.  I think 
> this is a hold over from the pre-I/O thread days where we selectively 
> set SIGIO on certain file descriptors to make sure that when an IO fd 
> became readable, we received a signal to break out of the KVM emulation 
> loop.
> 
> Can the folks on CC confirm/deny?
> 
> I can't see any use of SIGIO in the current source tree.

At least qemu-timer.c uses SIGIO in HPET mode. That only applies to
Linux hosts, though.

Jan
Avi Kivity May 31, 2011, 2:11 p.m. UTC | #3
On 05/31/2011 04:47 PM, Anthony Liguori wrote:
> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>> and doesn't need to be blocked.
>>
>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>
> Anything to do with signal masks is never a trivial patch BTW...
>
> But I actually think explicit handling of SIGIO is unneeded.  I think 
> this is a hold over from the pre-I/O thread days where we selectively 
> set SIGIO on certain file descriptors to make sure that when an IO fd 
> became readable, we received a signal to break out of the KVM 
> emulation loop.
>
> Can the folks on CC confirm/deny?
>
> I can't see any use of SIGIO in the current source tree.
>

We have O_ASYNC in enable_sigio_timer().  That's only used with HPET 
host timers, which should be very rare.

To be on the safe side I think we should take in the patch, and drop 
SIGIO support completely only if we decide the hpet host timer is not 
worth supporting.
Anthony Liguori May 31, 2011, 2:26 p.m. UTC | #4
On 05/31/2011 09:06 AM, Jan Kiszka wrote:
> On 2011-05-31 15:47, Anthony Liguori wrote:
>> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>>> and doesn't need to be blocked.
>>>
>>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>>
>> Anything to do with signal masks is never a trivial patch BTW...
>>
>> But I actually think explicit handling of SIGIO is unneeded.  I think
>> this is a hold over from the pre-I/O thread days where we selectively
>> set SIGIO on certain file descriptors to make sure that when an IO fd
>> became readable, we received a signal to break out of the KVM emulation
>> loop.
>>
>> Can the folks on CC confirm/deny?
>>
>> I can't see any use of SIGIO in the current source tree.
>
> At least qemu-timer.c uses SIGIO in HPET mode. That only applies to
> Linux hosts, though.

Is there any reason we still carry multiple timer implementations these 
days?

HPET shouldn't be any better than dynticks.

Regards,

Anthony Liguori

>
> Jan
>
Jan Kiszka May 31, 2011, 2:54 p.m. UTC | #5
On 2011-05-31 16:26, Anthony Liguori wrote:
> On 05/31/2011 09:06 AM, Jan Kiszka wrote:
>> On 2011-05-31 15:47, Anthony Liguori wrote:
>>> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>>>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>>>> and doesn't need to be blocked.
>>>>
>>>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>>>
>>> Anything to do with signal masks is never a trivial patch BTW...
>>>
>>> But I actually think explicit handling of SIGIO is unneeded.  I think
>>> this is a hold over from the pre-I/O thread days where we selectively
>>> set SIGIO on certain file descriptors to make sure that when an IO fd
>>> became readable, we received a signal to break out of the KVM emulation
>>> loop.
>>>
>>> Can the folks on CC confirm/deny?
>>>
>>> I can't see any use of SIGIO in the current source tree.
>>
>> At least qemu-timer.c uses SIGIO in HPET mode. That only applies to
>> Linux hosts, though.
> 
> Is there any reason we still carry multiple timer implementations these 
> days?
> 
> HPET shouldn't be any better than dynticks.

On any recent kernel, for sure. BTW, the same applies to the RTC timer.

Jan
Alexander Graf May 31, 2011, 3:44 p.m. UTC | #6
On 31.05.2011, at 16:54, Jan Kiszka wrote:

> On 2011-05-31 16:26, Anthony Liguori wrote:
>> On 05/31/2011 09:06 AM, Jan Kiszka wrote:
>>> On 2011-05-31 15:47, Anthony Liguori wrote:
>>>> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>>>>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>>>>> and doesn't need to be blocked.
>>>>> 
>>>>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>>>> 
>>>> Anything to do with signal masks is never a trivial patch BTW...
>>>> 
>>>> But I actually think explicit handling of SIGIO is unneeded.  I think
>>>> this is a hold over from the pre-I/O thread days where we selectively
>>>> set SIGIO on certain file descriptors to make sure that when an IO fd
>>>> became readable, we received a signal to break out of the KVM emulation
>>>> loop.
>>>> 
>>>> Can the folks on CC confirm/deny?
>>>> 
>>>> I can't see any use of SIGIO in the current source tree.
>>> 
>>> At least qemu-timer.c uses SIGIO in HPET mode. That only applies to
>>> Linux hosts, though.
>> 
>> Is there any reason we still carry multiple timer implementations these 
>> days?
>> 
>> HPET shouldn't be any better than dynticks.
> 
> On any recent kernel, for sure. BTW, the same applies to the RTC timer.

So the obvious change would be to introduce CONFIG_HPET, ifdef the SIGIO handling on that and also ifdef the host hpet handling code on it? That way it's documented well and can preferably even be turned off with --disable-host-hpet during configure time, which we can then slowly turn to the default.


Alex
Anthony Liguori May 31, 2011, 3:48 p.m. UTC | #7
On 05/31/2011 10:44 AM, Alexander Graf wrote:
>
> On 31.05.2011, at 16:54, Jan Kiszka wrote:
>
>> On 2011-05-31 16:26, Anthony Liguori wrote:
>>> On 05/31/2011 09:06 AM, Jan Kiszka wrote:
>>>> On 2011-05-31 15:47, Anthony Liguori wrote:
>>>>> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>>>>>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>>>>>> and doesn't need to be blocked.
>>>>>>
>>>>>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>>>>>
>>>>> Anything to do with signal masks is never a trivial patch BTW...
>>>>>
>>>>> But I actually think explicit handling of SIGIO is unneeded.  I think
>>>>> this is a hold over from the pre-I/O thread days where we selectively
>>>>> set SIGIO on certain file descriptors to make sure that when an IO fd
>>>>> became readable, we received a signal to break out of the KVM emulation
>>>>> loop.
>>>>>
>>>>> Can the folks on CC confirm/deny?
>>>>>
>>>>> I can't see any use of SIGIO in the current source tree.
>>>>
>>>> At least qemu-timer.c uses SIGIO in HPET mode. That only applies to
>>>> Linux hosts, though.
>>>
>>> Is there any reason we still carry multiple timer implementations these
>>> days?
>>>
>>> HPET shouldn't be any better than dynticks.
>>
>> On any recent kernel, for sure. BTW, the same applies to the RTC timer.
>
> So the obvious change would be to introduce CONFIG_HPET, ifdef the SIGIO handling on that and also ifdef the host hpet handling code on it? That way it's documented well and can preferably even be turned off with --disable-host-hpet during configure time, which we can then slowly turn to the default.

Or just remove hpet and rtc.

Does anyone really object to that?

Regards,

Anthony Liguori

>
> Alex
>
>
Alexander Graf May 31, 2011, 4:16 p.m. UTC | #8
On 31.05.2011, at 17:48, Anthony Liguori wrote:

> On 05/31/2011 10:44 AM, Alexander Graf wrote:
>> 
>> On 31.05.2011, at 16:54, Jan Kiszka wrote:
>> 
>>> On 2011-05-31 16:26, Anthony Liguori wrote:
>>>> On 05/31/2011 09:06 AM, Jan Kiszka wrote:
>>>>> On 2011-05-31 15:47, Anthony Liguori wrote:
>>>>>> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>>>>>>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>>>>>>> and doesn't need to be blocked.
>>>>>>> 
>>>>>>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>>>>>> 
>>>>>> Anything to do with signal masks is never a trivial patch BTW...
>>>>>> 
>>>>>> But I actually think explicit handling of SIGIO is unneeded.  I think
>>>>>> this is a hold over from the pre-I/O thread days where we selectively
>>>>>> set SIGIO on certain file descriptors to make sure that when an IO fd
>>>>>> became readable, we received a signal to break out of the KVM emulation
>>>>>> loop.
>>>>>> 
>>>>>> Can the folks on CC confirm/deny?
>>>>>> 
>>>>>> I can't see any use of SIGIO in the current source tree.
>>>>> 
>>>>> At least qemu-timer.c uses SIGIO in HPET mode. That only applies to
>>>>> Linux hosts, though.
>>>> 
>>>> Is there any reason we still carry multiple timer implementations these
>>>> days?
>>>> 
>>>> HPET shouldn't be any better than dynticks.
>>> 
>>> On any recent kernel, for sure. BTW, the same applies to the RTC timer.
>> 
>> So the obvious change would be to introduce CONFIG_HPET, ifdef the SIGIO handling on that and also ifdef the host hpet handling code on it? That way it's documented well and can preferably even be turned off with --disable-host-hpet during configure time, which we can then slowly turn to the default.
> 
> Or just remove hpet and rtc.
> 
> Does anyone really object to that?

Do RHEL5 and SLES10 support dynticks? If yes, no objections. They're the oldest really supported distros we should possibly remotely even care about.


Alex
Anthony Liguori May 31, 2011, 7:49 p.m. UTC | #9
On 05/31/2011 11:16 AM, Alexander Graf wrote:
>
> On 31.05.2011, at 17:48, Anthony Liguori wrote:
>
>> On 05/31/2011 10:44 AM, Alexander Graf wrote:
>>>
>>> On 31.05.2011, at 16:54, Jan Kiszka wrote:
>>>
>>>> On 2011-05-31 16:26, Anthony Liguori wrote:
>>>>> On 05/31/2011 09:06 AM, Jan Kiszka wrote:
>>>>>> On 2011-05-31 15:47, Anthony Liguori wrote:
>>>>>>> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>>>>>>>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>>>>>>>> and doesn't need to be blocked.
>>>>>>>>
>>>>>>>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>>>>>>>
>>>>>>> Anything to do with signal masks is never a trivial patch BTW...
>>>>>>>
>>>>>>> But I actually think explicit handling of SIGIO is unneeded.  I think
>>>>>>> this is a hold over from the pre-I/O thread days where we selectively
>>>>>>> set SIGIO on certain file descriptors to make sure that when an IO fd
>>>>>>> became readable, we received a signal to break out of the KVM emulation
>>>>>>> loop.
>>>>>>>
>>>>>>> Can the folks on CC confirm/deny?
>>>>>>>
>>>>>>> I can't see any use of SIGIO in the current source tree.
>>>>>>
>>>>>> At least qemu-timer.c uses SIGIO in HPET mode. That only applies to
>>>>>> Linux hosts, though.
>>>>>
>>>>> Is there any reason we still carry multiple timer implementations these
>>>>> days?
>>>>>
>>>>> HPET shouldn't be any better than dynticks.
>>>>
>>>> On any recent kernel, for sure. BTW, the same applies to the RTC timer.
>>>
>>> So the obvious change would be to introduce CONFIG_HPET, ifdef the SIGIO handling on that and also ifdef the host hpet handling code on it? That way it's documented well and can preferably even be turned off with --disable-host-hpet during configure time, which we can then slowly turn to the default.
>>
>> Or just remove hpet and rtc.
>>
>> Does anyone really object to that?
>
> Do RHEL5 and SLES10 support dynticks? If yes, no objections. They're the oldest really supported distros we should possibly remotely even care about.

Yes, they do.  But it's not as accurate as RTC/HPET because there is no 
CONFIG_HRTIMERS.

But the problem with RTC/HPET is that there is only one /dev/rtc and one 
/dev/hpet so only one guest can use it at any given time.  It's really 
not a generally useful solution.

At one point in time, it was the only way to get a high res clock.  Now, 
it Just Works provided you don't have an ancient kernel.

Regards,

Anthony Liguori
Andreas Färber May 31, 2011, 9:11 p.m. UTC | #10
Am 31.05.2011 um 21:49 schrieb Anthony Liguori:

> On 05/31/2011 11:16 AM, Alexander Graf wrote:
>>
>> On 31.05.2011, at 17:48, Anthony Liguori wrote:
>>
>>> On 05/31/2011 10:44 AM, Alexander Graf wrote:
>>>>
>>>> On 31.05.2011, at 16:54, Jan Kiszka wrote:
>>>>
>>>>> On 2011-05-31 16:26, Anthony Liguori wrote:
>>>>>> Is there any reason we still carry multiple timer  
>>>>>> implementations these
>>>>>> days?
>>>>>>
>>>>>> HPET shouldn't be any better than dynticks.
>>>>>
>>>>> On any recent kernel, for sure. BTW, the same applies to the RTC  
>>>>> timer.
>>>>
>>>> So the obvious change would be to introduce CONFIG_HPET, ifdef  
>>>> the SIGIO handling on that and also ifdef the host hpet handling  
>>>> code on it? That way it's documented well and can preferably even  
>>>> be turned off with --disable-host-hpet during configure time,  
>>>> which we can then slowly turn to the default.
>>>
>>> Or just remove hpet and rtc.
>>>
>>> Does anyone really object to that?

--verbose please: We're not talking about "removal" of emulation of  
such acronyms for i386 guests, but about ceasing to use some Linux- 
only host facilities, right?

>> Do RHEL5 and SLES10 support dynticks? If yes, no objections.  
>> They're the oldest really supported distros we should possibly  
>> remotely even care about.
>
> Yes, they do.  But it's not as accurate as RTC/HPET because there is  
> no CONFIG_HRTIMERS.
>
> But the problem with RTC/HPET is that there is only one /dev/rtc and  
> one /dev/hpet so only one guest can use it at any given time.  It's  
> really not a generally useful solution.
>
> At one point in time, it was the only way to get a high res clock.   
> Now, it Just Works provided you don't have an ancient kernel.

I'm curious, what's ancient these days? 2.6.29 or more like 2.4.x?

Andreas
Andreas Färber May 31, 2011, 9:24 p.m. UTC | #11
Am 31.05.2011 um 15:47 schrieb Anthony Liguori:

> On 05/29/2011 04:50 PM, Andreas Färber wrote:
>> BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
>> and doesn't need to be blocked.
>>
>> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
>
> Anything to do with signal masks is never a trivial patch BTW...

The way I saw it, it didn't change signal behavior on Linux, Darwin,  
BSD, etc. hosts (where SIGIO must've been defined) and trivially  
unbroke not-yet-fully-supported Haiku host.

Andreas
Jan Kiszka May 31, 2011, 9:43 p.m. UTC | #12
On 2011-05-31 23:11, Andreas Färber wrote:
> Am 31.05.2011 um 21:49 schrieb Anthony Liguori:
> 
>> On 05/31/2011 11:16 AM, Alexander Graf wrote:
>>>
>>> On 31.05.2011, at 17:48, Anthony Liguori wrote:
>>>
>>>> On 05/31/2011 10:44 AM, Alexander Graf wrote:
>>>>>
>>>>> On 31.05.2011, at 16:54, Jan Kiszka wrote:
>>>>>
>>>>>> On 2011-05-31 16:26, Anthony Liguori wrote:
>>>>>>> Is there any reason we still carry multiple timer implementations
>>>>>>> these
>>>>>>> days?
>>>>>>>
>>>>>>> HPET shouldn't be any better than dynticks.
>>>>>>
>>>>>> On any recent kernel, for sure. BTW, the same applies to the RTC
>>>>>> timer.
>>>>>
>>>>> So the obvious change would be to introduce CONFIG_HPET, ifdef the
>>>>> SIGIO handling on that and also ifdef the host hpet handling code
>>>>> on it? That way it's documented well and can preferably even be
>>>>> turned off with --disable-host-hpet during configure time, which we
>>>>> can then slowly turn to the default.
>>>>
>>>> Or just remove hpet and rtc.
>>>>
>>>> Does anyone really object to that?
> 
> --verbose please: We're not talking about "removal" of emulation of such
> acronyms for i386 guests, but about ceasing to use some Linux-only host
> facilities, right?

Right.

> 
>>> Do RHEL5 and SLES10 support dynticks? If yes, no objections. They're
>>> the oldest really supported distros we should possibly remotely even
>>> care about.
>>
>> Yes, they do.  But it's not as accurate as RTC/HPET because there is
>> no CONFIG_HRTIMERS.
>>
>> But the problem with RTC/HPET is that there is only one /dev/rtc and
>> one /dev/hpet so only one guest can use it at any given time.  It's
>> really not a generally useful solution.
>>
>> At one point in time, it was the only way to get a high res clock. 
>> Now, it Just Works provided you don't have an ancient kernel.
> 
> I'm curious, what's ancient these days? 2.6.29 or more like 2.4.x?

IIRC, highres timers started to work around 2.6.24 on x86. Anyone on
such an old kernel is likely also not interested in updating QEMU.

Jan
Anthony Liguori May 31, 2011, 11:19 p.m. UTC | #13
On 05/31/2011 04:43 PM, Jan Kiszka wrote:
> On 2011-05-31 23:11, Andreas Färber wrote:
>> Am 31.05.2011 um 21:49 schrieb Anthony Liguori:
>>> At one point in time, it was the only way to get a high res clock.
>>> Now, it Just Works provided you don't have an ancient kernel.
>>
>> I'm curious, what's ancient these days? 2.6.29 or more like 2.4.x?
>
> IIRC, highres timers started to work around 2.6.24 on x86. Anyone on
> such an old kernel is likely also not interested in updating QEMU.

Yeah, 2.6.24 is ancient now that we're in the 3.0 world ;-)

Regards,

Anthony Liguori

>
> Jan
>
Stefan Hajnoczi June 1, 2011, 10:30 p.m. UTC | #14
On Tue, May 31, 2011 at 08:47:25AM -0500, Anthony Liguori wrote:
> On 05/29/2011 04:50 PM, Andreas Färber wrote:
> >BeOS and Haiku don't define SIGIO. When undefined, it won't arrive
> >and doesn't need to be blocked.
> >
> >Signed-off-by: Andreas Färber<andreas.faerber@web.de>
> 
> Anything to do with signal masks is never a trivial patch BTW...

If you'll take this patch directly then I'll drop it off the radar.

Stefan
diff mbox

Patch

diff --git a/cpus.c b/cpus.c
index 1fc34b7..42af291 100644
--- a/cpus.c
+++ b/cpus.c
@@ -395,7 +395,9 @@  static int qemu_signal_init(void)
     pthread_sigmask(SIG_UNBLOCK, &set, NULL);
 
     sigemptyset(&set);
+#ifdef SIGIO
     sigaddset(&set, SIGIO);
+#endif
     sigaddset(&set, SIGALRM);
     sigaddset(&set, SIG_IPI);
     sigaddset(&set, SIGBUS);
@@ -408,7 +410,9 @@  static int qemu_signal_init(void)
          * We need to process timer signals synchronously to avoid a race
          * between exit_request check and KVM vcpu entry.
          */
+#ifdef SIGIO
         sigaddset(&set, SIGIO);
+#endif
         sigaddset(&set, SIGALRM);
     }
 #endif
@@ -449,12 +453,16 @@  static void qemu_kvm_init_cpu_signals(CPUState *env)
 #else
     sigemptyset(&set);
     sigaddset(&set, SIG_IPI);
+#ifdef SIGIO
     sigaddset(&set, SIGIO);
+#endif
     sigaddset(&set, SIGALRM);
     pthread_sigmask(SIG_BLOCK, &set, NULL);
 
     pthread_sigmask(SIG_BLOCK, NULL, &set);
+#ifdef SIGIO
     sigdelset(&set, SIGIO);
+#endif
     sigdelset(&set, SIGALRM);
 #endif
     sigdelset(&set, SIG_IPI);