Re: [PATCH 1/1] intel8x0: Improve performance in virtual environment

Re: [PATCH 1/1] intel8x0: Improve performance in virtual environment

[Jaroslav Kysela]

Date 20.10.2011 17:02, Konstantin Ozerkov wrote:
> This patch intended to improve performance in virtualized environments
> like Parallels Desktop or VirtualBox/QEMU (virtual ICH/AC97 audio).
> 
> I/O access is very time-expensive operation in virtual world: VCPU
> can be rescheduled and in the worst case we got more than 10ms delay on
> each I/O access.
> 
> In the original code normal loop exit rule
> (old_civ == current_civ && old_picb == current_picb) was never satisfied,
> because old_picb was never the same as current_picb due delay inspired by
> reading current_civ. As a result loop ended by timeout and we got 10x more
> I/O operations.
> 
> To prevent glitch on buffer swapping the rule (old_civ == current_civ) is
> enough.

I don't think so. This code was added because we got some strange
situations (values) with the real intel8x0 hardware. A better way might
be to identify the virtualisation (maybe using special PCI IDs) and skip
these extra checks in the driver for these devices.

NAK from me.

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH 1/1] intel8x0: Improve performance in virtual environment

[Takashi Iwai]

At Thu, 20 Oct 2011 19:02:50 +0400,
Konstantin Ozerkov wrote:
> 
> This patch intended to improve performance in virtualized environments
> like Parallels Desktop or VirtualBox/QEMU (virtual ICH/AC97 audio).
> 
> I/O access is very time-expensive operation in virtual world: VCPU
> can be rescheduled and in the worst case we got more than 10ms delay on
> each I/O access.
> 
> In the original code normal loop exit rule
> (old_civ == current_civ && old_picb == current_picb) was never satisfied,
> because old_picb was never the same as current_picb due delay inspired by
> reading current_civ. As a result loop ended by timeout and we got 10x more
> I/O operations.

You mean in the virtual environment, right?  On the real machines,
this condition can be satisfied normally because the read is much
faster than the sample rate.  This check is needed to avoid the broken
behavior around the boundary.

> To prevent glitch on buffer swapping the rule (old_civ == current_civ) is
> enough.

Not for real (thus less logical) machines, unfortunately.  Both checks
were needed in the real tests.

I don't mind to implement some bypass if the driver knows that it's
running in a VM.  But, removing blindly would cause definitely a
regression.


thanks,

Takashi


> Experimental data from Prallels Desktop 7, RHEL6 guest (I/O ops per second):
> 
> Original code:
> In Port    Counter         Callback
>    f014      41550         fffff00000179d00 ac97_bm_read_civ+0x000
>    f018      41387         fffff0000017a580 ac97_bm_read_picb+0x000
> 
> With patch:
> In Port    Counter         Callback
>    f014       4090         fffff00000179d00 ac97_bm_read_civ+0x000
>    f018       1964         fffff0000017a580 ac97_bm_read_picb+0x000
> 
> Signed-off-by: Konstantin Ozerkov <kozerkov@parallels.com>
> Signed-off-by: Denis V. Lunev <den@parallels.com>
> 
> diff --git a/pci/intel8x0.c b/pci/intel8x0.c
> index 6a5b387..bad4a65 100644
> --- a/pci/intel8x0.c
> +++ b/pci/intel8x0.c
> @@ -1065,8 +1065,7 @@ static snd_pcm_uframes_t snd_intel8x0_pcm_pointer(struct snd_pcm_substream *subs
>  			udelay(10);
>  			continue;
>  		}
> -		if (civ == igetbyte(chip, ichdev->reg_offset + ICH_REG_OFF_CIV) &&
> -		    ptr1 == igetword(chip, ichdev->reg_offset + ichdev->roff_picb))
> +		if (civ == igetbyte(chip, ichdev->reg_offset + ICH_REG_OFF_CIV))
>  			break;
>  	} while (timeout--);
>  	ptr = ichdev->last_pos;
> -- 
> 1.7.7
> 

Re: [PATCH 1/1] intel8x0: Improve performance in virtual environment

[Takashi Iwai]

At Thu, 20 Oct 2011 21:32:38 +0400,
Konstantin Ozerkov wrote:
> 
> Thank you for fast response and comments.
> 
> I will rewrite this patch according you suggestions:
> * add VM environment detection logic (PD only, I don't have clear vision how to
> that for VirtualBox/QEMU) and module parameter (for manual control)
> * make separated VM-optimized variant of snd_intel8x0_pcm_pointer()

Sounds reasonable.

> Initially this patch is result of research and profiling audio emulation quality in Parallels Desktop.
>
> On 20.10.11 19:21, Takashi Iwai wrote:
> > At Thu, 20 Oct 2011 19:02:50 +0400,
> > Konstantin Ozerkov wrote:
> >> This patch intended to improve performance in virtualized environments
> >> like Parallels Desktop or VirtualBox/QEMU (virtual ICH/AC97 audio).
> >>
> >> I/O access is very time-expensive operation in virtual world: VCPU
> >> can be rescheduled and in the worst case we got more than 10ms delay on
> >> each I/O access.
> >>
> >> In the original code normal loop exit rule
> >> (old_civ == current_civ&&  old_picb == current_picb) was never satisfied,
> >> because old_picb was never the same as current_picb due delay inspired by
> >> reading current_civ. As a result loop ended by timeout and we got 10x more
> >> I/O operations.
> > You mean in the virtual environment, right?  On the real machines,
> > this condition can be satisfied normally because the read is much
> > faster than the sample rate.  This check is needed to avoid the broken
> > behavior around the boundary.
> >
> >> To prevent glitch on buffer swapping the rule (old_civ == current_civ) is
> >> enough.
> > Not for real (thus less logical) machines, unfortunately.  Both checks
> > were needed in the real tests.
> Can you say which hardware glitches this way and and which conditions?
> My simplified rule works well in many Windows AC97 drivers which I debug/analyze.

Some hardware showed the unstable CIV and PICB values when it goes
crossing the boundary.  Both registers should be updated
simultaneously, but in reality, you may hit at a bad timing where only
one of them is updated.  And the order of updates of CIV/PICB isn't
defined.  Actually, in the earlier version, we tested only CIV, but it
turned out that it's didn't suffice.  So, we had to check both
values.


thanks,

Takashi

Re: Is locking problem in snd_intel8x0_pcm_pointer() ?

[Takashi Iwai]

At Mon, 24 Oct 2011 15:48:44 +0400,
Konstantin Ozerkov wrote:
> 
> Seems that locking is broken: snd_intel8x0_pcm_pointer() uses spin_lock()/spin_unlock() semantic
> for lock which is shared in ISR ( snd_intel8x0_update). As result we got possible deadlock inside ISR
> on UP system. And needed special kludge inside snd_intel8x0_pcm_pointer() to prevent moving
> backward (really this is race with snd_intel8x0_update).

Hm, could you elaborate how can it a problem?
snd_intel8x0_update() releases the lock before calling
snd_pcm_period_update() that calls the pointer callback.


Takashi