Re: about in_multi_mtx _at_ netinet/in_mcast.c:1095

On Thu, Sep 09, 2010 at 09:36:19AM -0400, John Baldwin wrote:
> On Wednesday, September 08, 2010 4:14:19 pm Weongyo Jeong wrote:
> > Hello,
> > 
> > I have a question about IN_MULTI_LOCK() because it uses MTX_DEF flag
> > when it's initialized so I always encounters the following LOR
> > 
> > lock order reversal: (sleepable after non-sleepable)
> >  1st 0xffffffff80d0b560 in_multi_mtx (in_multi_mtx) _at_ 
> netinet/in_mcast.c:1095
> >  2nd 0xffffff00014e3850 USB device SX lock (USB device SX lock) _at_ 
> /usr/home/freebsd_usb/sys/modules/usb/usb/../../../dev/usb/usb_request.c:441
> > KDB: stack backtrace:
> > db_trace_self_wrapper() at db_trace_self_wrapper+0x2a
> > _witness_debugger() at _witness_debugger+0x2e
> > witness_checkorder() at witness_checkorder+0x807
> > _sx_xlock() at _sx_xlock+0x55
> > usbd_do_request_flags() at usbd_do_request_flags+0xe5
> > axe_cmd() at axe_cmd+0xc7
> > axe_setmulti_locked() at axe_setmulti_locked+0x70
> > axe_setmulti() at axe_setmulti+0x3e
> > axe_ioctl() at axe_ioctl+0x132
> > if_addmulti() at if_addmulti+0x19b
> > in_joingroup_locked() at in_joingroup_locked+0x1bc
> > in_joingroup() at in_joingroup+0x52
> > in_control() at in_control+0x1144
> > ifioctl() at ifioctl+0x1118
> > kern_ioctl() at kern_ioctl+0xbe
> > ioctl() at ioctl+0xfd
> > syscallenter() at syscallenter+0x1aa
> > syscall() at syscall+0x4c
> > Xfast_syscall() at Xfast_syscall+0xe2
> > 
> > when I uses the following code at driver's ioctl routine:
> > 
> > 	case SIOCADDMULTI:
> > 	case SIOCDELMULTI:
> > 		axe_setmulti(sc, 0);
> > 		break;
> > 
> > It means that USB driver always should defer SIOCADDMULTI /
> > SIOCDELMULTI handling to the other process context to avoid LOR.
> > 
> > My question is that is it safe if the multicasting operations for USB
> > device happens without IN_MULTI_LOCK?  Or is there any race cases if the
> > task is deferred?
> 
> Why is USB using an sx lock instead of a mutex?

Frankly speaking I also don't know why hps_at_ uses sx lock.  That is one
of things I'd like to change it.

Just looking the comment at usb_request.c_at_441:

	/*
	 * Grab the default sx-lock so that serialisation
	 * is achieved when multiple threads are involved:
	 */
	sx_xlock(&udev->ctrl_sx);

I think he might want to hold the lock even if the thread is going into
sleep. It might be for serialization.

However even if we succeed to change the lock from sx to mutex, it's
hard to avoid the requests going into the sleep.  It means USB stack
should call like below:

	mtx_sleep(chan, IN_MULTI_LOCK, ...);

to avoid the kernel's complain (would be `sleep with holding
non-sleepable lock').

What I'd like to say is that the sleeping is big problem if mutex is
used that it'd be worse when multiple mutex locks are used.

So I'm looking for a fundamental solution to solve this problem.
Welcomes any ideas.

regards,
Weongyo Jeong