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

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.

-- 
John Baldwin