Re: 5.2-BETA dsp.c duplicate lock
On Dec 02, Mathew Kanner wrote: > On Dec 01, Maxime Henrion wrote: > [snip] > I believe that your patch should fix the problem. In general > I see one of three strategies, > > 1)Your patch, > 2)create a new snd_mtxcreate_chan for channels that sets the > flags DUP_OK. > 3)Fix locking to never hold duplicates. First glance suggests > that would be contained in dsp.c, the ioctl handler is the real > problem and seems inconsistent with itself in regards to locking. > Ugh. Why do the best ideas happen after you send? 4) Make read and write channel locks of a different class. --Mat -- In general, a standard is very useful, whether it's de facto or du jour. - Microsoft's Greg Sullivan as misquoted by News.Com ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: 5.2-BETA dsp.c duplicate lock
On Dec 01, Maxime Henrion wrote: > Mathew Kanner wrote: > [patch ripped] > > > > Maxime, > > I think it would be better to isolate the changes (DUP_OK flag > > and lock creation) to just the channel code, no need to touch every > > driver. > > Yes, but to do this I'd need either to make the channel code use > mtx_init() directly, which would defeat the purpose of the USING_MUTEX > define, or to completely unifdef -U it. Since I had no idea if this code > was actually used or not, I went the safe way and just changed the > snd_mtxcreate() wrapper interface to accept mutex options. For what it's > worth, there is at least one direct invokation of mtx_init() in the sound > drivers, so it seems this define is actually already broken. Funny, I was thinking the exact same thing the other day. I'm %100 for this idea. > This needs to be sorted out before committing this patch if we need to, > but for now, I just wanted to see if using MTX_DUPOK solved the problem or > not. I believe that your patch should fix the problem. In general I see one of three strategies, 1) Your patch, 2) create a new snd_mtxcreate_chan for channels that sets the flags DUP_OK. 3) Fix locking to never hold duplicates. First glance suggests that would be contained in dsp.c, the ioctl handler is the real problem and seems inconsistent with itself in regards to locking. Ugh. > > > Also, if this is the right direction, we should back out the > > commit I did that re-ordered code that prevented duplicate channel > > locks (obviously it wasn't completen) channel. > > If the code is not supposed to acquire several channel locks at once, > then yes, it's not the right direction to go.As I said in my previous > mail, and that's why I wanted the advice of you sound guys, in that case > it's just a workaround. :-) I hear you. The fact is, currently there are no experienced sound people. Anyway, my order of preference is #2, #3, #1. Since #1 exists, I don't mind it being committed until we really understand what's going on. I will work on #3 tonight and maybe fallback to #2. Cheers, --Mat -- Applicants must also have extensive knowledge of UNIX, although they should have sufficiently good programming taste to not consider this an achievement. - MIT AI Lab job ad in the /Boston Globe/ ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: 5.2-BETA dsp.c duplicate lock
Maxime Henrion wrote: > Mathew Kanner wrote: > [patch ripped] >> >> Maxime, >> I think it would be better to isolate the changes (DUP_OK flag >> and lock creation) to just the channel code, no need to touch every >> driver. > > Yes, but to do this I'd need either to make the channel code use > mtx_init() directly, which would defeat the purpose of the USING_MUTEX > define, or to completely unifdef -U it. Since I had no idea if this code > was actually used or not, I went the safe way and just changed the > snd_mtxcreate() wrapper interface to accept mutex options. For what it's > worth, there is at least one direct invokation of mtx_init() in the sound > drivers, so it seems this define is actually already broken. > > This needs to be sorted out before committing this patch if we need to, > but for now, I just wanted to see if using MTX_DUPOK solved the problem or > not. I'll try to test later today. Thanks! -- Jesse Guardiani, Systems Administrator WingNET Internet Services, P.O. Box 2605 // Cleveland, TN 37320-2605 423-559-LINK (v) 423-559-5145 (f) http://www.wingnet.net ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: 5.2-BETA dsp.c duplicate lock
Mathew Kanner wrote: [patch ripped] > > Maxime, > I think it would be better to isolate the changes (DUP_OK flag > and lock creation) to just the channel code, no need to touch every > driver. Yes, but to do this I'd need either to make the channel code use mtx_init() directly, which would defeat the purpose of the USING_MUTEX define, or to completely unifdef -U it. Since I had no idea if this code was actually used or not, I went the safe way and just changed the snd_mtxcreate() wrapper interface to accept mutex options. For what it's worth, there is at least one direct invokation of mtx_init() in the sound drivers, so it seems this define is actually already broken. This needs to be sorted out before committing this patch if we need to, but for now, I just wanted to see if using MTX_DUPOK solved the problem or not. > Also, if this is the right direction, we should back out the > commit I did that re-ordered code that prevented duplicate channel > locks (obviously it wasn't completen) channel. If the code is not supposed to acquire several channel locks at once, then yes, it's not the right direction to go. As I said in my previous mail, and that's why I wanted the advice of you sound guys, in that case it's just a workaround. :-) Cheers, Maxime ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: 5.2-BETA dsp.c duplicate lock
On Dec 01, Maxime Henrion wrote:
> Jesse Guardiani wrote:
> > Jesse Guardiani wrote:
> Index: isa/ad1816.c
> ===
> RCS file: /space2/ncvs/src/sys/dev/sound/isa/ad1816.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 ad1816.c
> --- isa/ad1816.c 7 Sep 2003 16:28:02 - 1.29
> +++ isa/ad1816.c 1 Dec 2003 14:11:45 -
> @@ -593,7 +593,8 @@ ad1816_attach(device_t dev)
> ad1816 = (struct ad1816_info *)malloc(sizeof *ad1816, M_DEVBUF, M_NOWAIT |
> M_ZERO);
> if (!ad1816) return ENXIO;
>
> - ad1816->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
> + ad1816->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc",
> + 0);
> ad1816->io_rid = 2;
> ad1816->irq_rid = 0;
> ad1816->drq1_rid = 0;
> Index: isa/mss.c
> ===
> RCS file: /space2/ncvs/src/sys/dev/sound/isa/mss.c,v
> retrieving revision 1.86
> diff -u -p -r1.86 mss.c
> --- isa/mss.c 7 Sep 2003 16:28:02 - 1.86
> +++ isa/mss.c 1 Dec 2003 14:11:53 -
> @@ -1667,7 +1667,7 @@ mss_doattach(device_t dev, struct mss_in
> int pdma, rdma, flags = device_get_flags(dev);
> char status[SND_STATUSLEN], status2[SND_STATUSLEN];
>
> - mss->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
> + mss->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
> mss->bufsize = pcm_getbuffersize(dev, 4096, MSS_DEFAULT_BUFSZ, 65536);
> if (!mss_alloc_resources(mss, dev)) goto no;
> mss_init(mss, dev);
> Index: isa/sbc.c
> ===
> RCS file: /space2/ncvs/src/sys/dev/sound/isa/sbc.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 sbc.c
> --- isa/sbc.c 7 Feb 2003 14:05:33 - 1.38
> +++ isa/sbc.c 1 Dec 2003 14:12:03 -
> @@ -116,7 +116,8 @@ static void sb_setmixer(struct resource
> static void
> sbc_lockinit(struct sbc_softc *scp)
> {
> - scp->lock = snd_mtxcreate(device_get_nameunit(scp->dev), "sound softc");
> + scp->lock = snd_mtxcreate(device_get_nameunit(scp->dev), "sound softc",
> + 0);
> }
>
> static void
> Index: pci/cmi.c
> ===
> RCS file: /space2/ncvs/src/sys/dev/sound/pci/cmi.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 cmi.c
> --- pci/cmi.c 2 Sep 2003 17:30:37 - 1.23
> +++ pci/cmi.c 1 Dec 2003 14:12:15 -
> @@ -842,7 +842,7 @@ cmi_attach(device_t dev)
> return ENXIO;
> }
>
> - sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
> + sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
> data = pci_read_config(dev, PCIR_COMMAND, 2);
> data |= (PCIM_CMD_PORTEN|PCIM_CMD_BUSMASTEREN);
> pci_write_config(dev, PCIR_COMMAND, data, 2);
> Index: pci/ds1.c
> ===
> RCS file: /space2/ncvs/src/sys/dev/sound/pci/ds1.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 ds1.c
> --- pci/ds1.c 2 Sep 2003 17:30:37 - 1.36
> +++ pci/ds1.c 1 Dec 2003 14:12:26 -
> @@ -942,7 +945,7 @@ ds_pci_attach(device_t dev)
> return ENXIO;
> }
>
> - sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
> + sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
> sc->dev = dev;
> subdev = (pci_get_subdevice(dev) << 16) | pci_get_subvendor(dev);
> sc->type = ds_finddev(pci_get_devid(dev), subdev);
> Index: pci/emu10k1.c
> ===
> RCS file: /space2/ncvs/src/sys/dev/sound/pci/emu10k1.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 emu10k1.c
> --- pci/emu10k1.c 7 Sep 2003 16:28:03 - 1.41
> +++ pci/emu10k1.c 1 Dec 2003 14:12:35 -
> @@ -1468,7 +1468,7 @@ emu_pci_attach(device_t dev)
> return ENXIO;
> }
>
> - sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
> + sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
> sc->dev = dev;
> sc->type = pci_get_devid(dev);
> sc->rev = pci_get_revid(dev);
> Index: pci/t4dwave.c
> ===
> RCS file: /space2/ncvs/src/sys/dev/sound/pci/t4dwave.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 t4dwave.c
> --- pci/t4dwave.c 7 Sep 2003 16:28:03 - 1.40
> +++ pci/t4dwave.c 1 Dec 2003 14:12:42 -
> @@ -811,7 +811,7 @@ tr_pci_attach(device_t dev)
>
> tr->type = pci_get_devid(dev);
> tr->rev = pci_get_revid(dev);
> - tr->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
> + tr->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
>
> data = pci_read_config(dev, PCIR_COMMAND, 2);
> data |= (PC
Re: 5.2-BETA dsp.c duplicate lock
Jesse Guardiani wrote:
> Jesse Guardiani wrote:
>
> > I get this every time I `startx`. I didn't see it in
> > the archive either:
> >
> >
> > acquiring duplicate lock of same type: "pcm channel"
> > 1st pcm0:record:0 @ /usr/src/sys/dev/sound/pcm/dsp.c:144
> > 2nd pcm0:virtual:0 @ /usr/src/sys/dev/sound/pcm/dsp.c:146
> > Stack backtrace:
> > backtrace(c089b7e5,c3c96a54,c0a8f35a,92,200246) at backtrace+0x17
> > witness_lock(c3c6aa80,8,c0a8f35a,92,2002) at witness_lock+0x672
> > _mtx_lock_flags(c3c6aa80,0,c0a8f35a,92,c4) at _mtx_lock_flags+0xba
> > getchns(c3d07700,e473faf0,e473faf4,3000,c3b86980) at getchns+0x1b5
> > dsp_poll(c3d07700,c4,c403e640,c097fce0,0) at dsp_poll+0x46
> > spec_poll(e473fb48,e473fb68,c06d600c,e473fb48,c0937ca0) at spec_poll+0x180
> > spec_vnoperate(e473fb48,c0937ca0,c438db2c,c4,c4361a80) at
> > spec_vnoperate+0x18 vn_poll(c4599110,c4,c4361a80,c403e640,c4361a80) at
> > vn_poll+0x3c pollscan(c403e640,e473fbd8,3,3e1,18) at pollscan+0xb3
> > poll(c403e640,e473fd14,c08b631d,3ee,3) at poll+0x252
> > syscall(2f,2f,2f,,bfbfe748) at syscall+0x2c0
> > Xint0x80_syscall() at Xint0x80_syscall+0x1d
> > --- syscall (209), eip = 0x2869322f, esp = 0xbfbfe70c, ebp = 0xbfbfe768
> > ---
> >
>
> I this a known LOR? Or do I need to submit a pr?
This isn't a LOR at all. Could you please try the attached patch? It
should fix your problem.
To the sound guys : I believe it's expected that the pcm code will
sometimes acquire several channels lock at once, but I am not 100% sure.
If it's not, this patch isn't a fix but a workaround.
Cheers,
Maxime
Index: isa/ad1816.c
===
RCS file: /space2/ncvs/src/sys/dev/sound/isa/ad1816.c,v
retrieving revision 1.29
diff -u -p -r1.29 ad1816.c
--- isa/ad1816.c7 Sep 2003 16:28:02 - 1.29
+++ isa/ad1816.c1 Dec 2003 14:11:45 -
@@ -593,7 +593,8 @@ ad1816_attach(device_t dev)
ad1816 = (struct ad1816_info *)malloc(sizeof *ad1816, M_DEVBUF, M_NOWAIT |
M_ZERO);
if (!ad1816) return ENXIO;
- ad1816->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
+ ad1816->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc",
+ 0);
ad1816->io_rid = 2;
ad1816->irq_rid = 0;
ad1816->drq1_rid = 0;
Index: isa/mss.c
===
RCS file: /space2/ncvs/src/sys/dev/sound/isa/mss.c,v
retrieving revision 1.86
diff -u -p -r1.86 mss.c
--- isa/mss.c 7 Sep 2003 16:28:02 - 1.86
+++ isa/mss.c 1 Dec 2003 14:11:53 -
@@ -1667,7 +1667,7 @@ mss_doattach(device_t dev, struct mss_in
int pdma, rdma, flags = device_get_flags(dev);
char status[SND_STATUSLEN], status2[SND_STATUSLEN];
- mss->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
+ mss->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
mss->bufsize = pcm_getbuffersize(dev, 4096, MSS_DEFAULT_BUFSZ, 65536);
if (!mss_alloc_resources(mss, dev)) goto no;
mss_init(mss, dev);
Index: isa/sbc.c
===
RCS file: /space2/ncvs/src/sys/dev/sound/isa/sbc.c,v
retrieving revision 1.38
diff -u -p -r1.38 sbc.c
--- isa/sbc.c 7 Feb 2003 14:05:33 - 1.38
+++ isa/sbc.c 1 Dec 2003 14:12:03 -
@@ -116,7 +116,8 @@ static void sb_setmixer(struct resource
static void
sbc_lockinit(struct sbc_softc *scp)
{
- scp->lock = snd_mtxcreate(device_get_nameunit(scp->dev), "sound softc");
+ scp->lock = snd_mtxcreate(device_get_nameunit(scp->dev), "sound softc",
+ 0);
}
static void
Index: pci/cmi.c
===
RCS file: /space2/ncvs/src/sys/dev/sound/pci/cmi.c,v
retrieving revision 1.23
diff -u -p -r1.23 cmi.c
--- pci/cmi.c 2 Sep 2003 17:30:37 - 1.23
+++ pci/cmi.c 1 Dec 2003 14:12:15 -
@@ -842,7 +842,7 @@ cmi_attach(device_t dev)
return ENXIO;
}
- sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
+ sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
data = pci_read_config(dev, PCIR_COMMAND, 2);
data |= (PCIM_CMD_PORTEN|PCIM_CMD_BUSMASTEREN);
pci_write_config(dev, PCIR_COMMAND, data, 2);
Index: pci/ds1.c
===
RCS file: /space2/ncvs/src/sys/dev/sound/pci/ds1.c,v
retrieving revision 1.36
diff -u -p -r1.36 ds1.c
--- pci/ds1.c 2 Sep 2003 17:30:37 - 1.36
+++ pci/ds1.c 1 Dec 2003 14:12:26 -
@@ -942,7 +945,7 @@ ds_pci_attach(device_t dev)
return ENXIO;
}
- sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc");
+ sc->lock = snd_mtxcreate(device_get_nameunit(dev), "sound softc", 0);
sc->dev = dev;
subdev = (pci_get_subdevice(dev) << 16) | pci_ge
Re: 5.2-BETA dsp.c duplicate lock
Jesse Guardiani wrote: > I get this every time I `startx`. I didn't see it in > the archive either: > > > acquiring duplicate lock of same type: "pcm channel" > 1st pcm0:record:0 @ /usr/src/sys/dev/sound/pcm/dsp.c:144 > 2nd pcm0:virtual:0 @ /usr/src/sys/dev/sound/pcm/dsp.c:146 > Stack backtrace: > backtrace(c089b7e5,c3c96a54,c0a8f35a,92,200246) at backtrace+0x17 > witness_lock(c3c6aa80,8,c0a8f35a,92,2002) at witness_lock+0x672 > _mtx_lock_flags(c3c6aa80,0,c0a8f35a,92,c4) at _mtx_lock_flags+0xba > getchns(c3d07700,e473faf0,e473faf4,3000,c3b86980) at getchns+0x1b5 > dsp_poll(c3d07700,c4,c403e640,c097fce0,0) at dsp_poll+0x46 > spec_poll(e473fb48,e473fb68,c06d600c,e473fb48,c0937ca0) at spec_poll+0x180 > spec_vnoperate(e473fb48,c0937ca0,c438db2c,c4,c4361a80) at > spec_vnoperate+0x18 vn_poll(c4599110,c4,c4361a80,c403e640,c4361a80) at > vn_poll+0x3c pollscan(c403e640,e473fbd8,3,3e1,18) at pollscan+0xb3 > poll(c403e640,e473fd14,c08b631d,3ee,3) at poll+0x252 > syscall(2f,2f,2f,,bfbfe748) at syscall+0x2c0 > Xint0x80_syscall() at Xint0x80_syscall+0x1d > --- syscall (209), eip = 0x2869322f, esp = 0xbfbfe70c, ebp = 0xbfbfe768 > --- > I this a known LOR? Or do I need to submit a pr? -- Jesse Guardiani, Systems Administrator WingNET Internet Services, P.O. Box 2605 // Cleveland, TN 37320-2605 423-559-LINK (v) 423-559-5145 (f) http://www.wingnet.net ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
