Date: Tue, 27 Jul 2004 15:15:06 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: mat@cnd.mcgill.ca Cc: freebsd-current@FreeBSD.org Subject: Re: Questionable code in sys/dev/sound/pcm/channel.c Message-ID: <200407272215.i6RMF6jQ061016@gw.catspoiler.org> In-Reply-To: <20040727152327.GA92352@cnd.mcgill.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On 27 Jul, Mathew Kanner wrote:
> On Jul 26, John-Mark Gurney wrote:
>> Conrad J. Sabatier wrote this message on Mon, Jul 26, 2004 at 16:35 -0500:
>> > Why the formulaic calculation of timeout, if it's simply going to be
>> > unconditionally set to 1 immediately afterwards anyway? What's going on
>> > here?
>>
>> Well, if you look at the annotations, that absolute set of timeout was
>> added in rev 1.65 by cg with the comment:
>> tweaks to reduce latency/pauses in output
>>
>
>
> I think this has been raised on the mailling list before.
> IIRC, the logic for this is to check frequently for dead channels but
> CG is the authoriy.
My suspicion is that this change was made to reduce the consequences of
lost wakeups from the interrupt routine. This would have been more of a
problem when tsleep() was used in chn_sleep() and shouldn't be needed
now that the top and bottom halves of the code use the channel lock and
chn_sleep() uses msleep() to atomically release the lock and wait for
the wakeup from the interrupt code. That said, setting timeout to 1
shouldn't hurt anything and will just waste a bit of CPU time.
>> > Also, at the end of the function:
>> >
>> > if (count <= 0) {
>> > c->flags |= CHN_F_DEAD;
>> > printf("%s: play interrupt timeout, channel dead\n", c->name);
>> > }
>> >
>> > return ret;
>> > }
>>
>> that was changed in rev1.52 (by cg also), and previously was just a check
>> for count == 0..
>>
>> So, I'd recommend a message off to cg and ask why he made this changes...
The original version of the code always set timeout to 1 and looped on
(count > 0), so count could never go negative. When the code was
changed to set count to something larger than 1, count could go negative
if (hz % timeout != 0), so the condition for setting CHN_F_DEAD had to
be modified accordingly.
My suspicion is that there is sometimes enough latency in executing the
interrupt routine that the hardware DMA pointer is wrapping and
chn_dmaupdate() is calculating delta as zero. This would cause
chn_wrfeed() not to consume any data from the software buffer (and skip
the wakeup()), which might be enough to cause the chn_write() to time
out while waiting for space to become available in the software buffer.
It would be interesting to enable the debug code in chn_dmaupdate(), and
add (delta == 0) as a condition to trigger the device_printf().
The bigger question is what is the cause of the latency ...
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200407272215.i6RMF6jQ061016>