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

On Fri, Sep 10, 2010 at 05:17:39PM -0400, John Baldwin wrote:
> On Friday, September 10, 2010 4:28:58 pm Weongyo Jeong wrote:
> > 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.
> 
> It would introduce races in the multicast code to drop the lock around the 
> ioctl which would complicate it a good bit.  Non-USB ethernet drivers just use 
> plain locks which handle this situation just fine.

OK I see.  Thank you for explanation.

regards,
Weongyo Jeong