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

On Thu, Sep 09, 2010 at 02:32:52PM -0400, John Baldwin wrote:
> On Thursday, September 09, 2010 1:41:46 pm Weongyo Jeong wrote:
> > 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.
> 
> It is probably fine to just schedule a task to do the actual work of 
> axe_setmulti().  I think you do not need to lock IN_MULTI_LOCK yourself in 
> your task handler as long as your handler holds the appropriate lock 
> (if_maddr_rlock() IIRC) when walking the interface's multicast address list.

OK.  I'll try to use a task handler for this case.

One thing, just for curious.  Why the lower layer (in this case it might
be netinet/in_mcast.c) calls ioctl interface with holding IN_MULTI_LOCK?
If it calls ioctl without holding the lock, all drivers (specially all
USB drivers) which handles SIOCADDMULTI and SIOCDELMULTI don't need to
implement their own taskqueue or process context.

It looks to me that calling ioctl interface with holding IN_MULTI_LOCK
is useless if the drivers hold if_maddr_rlock(ifp) lock properly though
I could miss something important.

regards,
Weongyo Jeong