Re: netgraph(4) initialization order

Maksim Yevmenkin wrote:

> Dear Hackers,
> 
> any objections to the attached patch?
> 
> thanks,
> max
> 

Yes, as I stated in another email, I think that the core netgraph
module should be initialized before the SI_SUB_DRIVERS step.  I
propose creating a new sysinit called SI_SUB_NETGRAPH with a value
of 0x30100000.  That way it comes after SI_SUB_IF and before
SI_SUB_DRIVERS.  This make fiddling with SI_ORDER_* unneccesary.

Scott

> 
>>>>> Dear Hackers,
>>>>>
>>>>> would anyone object if i change SI_ORDER_MIDDLE in the 
>>>>> /sys/netgraph/ng_base.c:2994 to say SI_ORDER_THIRD, i.e.
>>>>>
>>>>> change
>>>>>
>>>>> DECLARE_MODULE(netgraph, netgraph_mod, SI_SUB_DRIVERS, 
>>>>> SI_ORDER_MIDDLE);
>>>>>
>>>>> to
>>>>>
>>>>> DECLARE_MODULE(netgraph, netgraph_mod, SI_SUB_DRIVERS,
>>>>> SI_ORDER_THIRD);
>>>>>
>>>>> the reason for this change is that bluetooth device drivers
>>>>> depend on netgraph(4) and when both netgraph(4) and bluetooth
>>>>> device driver (such as ng_ubt(4)) compiled in the kernel you
>>>>> get a crash. basically ng_ubt(4) mod_load callback is called
>>>>> before netgraph(4) mod_load callback and ng_findtype() crashes
>>>>> on uninitialized mutex (DEVICE_MODULE macro passes
>>>>> SI_SUB_DRIVERS, SI_ORDER_THIRD to the
>>>>
>>>>                  ^^^^^^^^^^^^^^ this should be SI_ORDER_MIDDLE :)
>>>
>>>
>>>>> DECLARE_MODULE).
>>>>
>>>>
>>>> I thought this is the task of MODULE_DEPEND.
>>>
>>>
>>> i thought so too :) but it appears to work only when module is 
>>> _loaded_ (by hand or from /boot/loader.conf), i.e. it does not work
>>> if module was compiled in the kernel.
>>
>>
>> maybe the config stuff could be extended to integrate the module 
>> dependency stuff along with the suggested order by moving things
>> backwards in the list if their dependencies suggest it.  (after the
>> bubble sort).
>>
>>>>> option #2 would be to have DEVICE_MODULE_ORDERED macro which
>>>>> accepts two extra parameters.
>>>>>
>>>>>
>>>>> and finally option #3 would be to duplicate entire content of
>>>>> the DEVICE_MODULE macro in all bluetooth device drivers and
>>>>> specify order in DECLARE_MODULE macro.
>>>>>
>>>>>
>>>>> any thoughts?
>>>>>
>>>>> thanks, max
> 
> 
> ------------------------------------------------------------------------
> 
> --- ng_base.c.orig	Wed Jan  5 12:04:36 2005
> +++ ng_base.c	Wed Jan  5 12:23:39 2005
> _at__at_ -46,6 +46,7 _at__at_
>  
>  #include <sys/param.h>
>  #include <sys/systm.h>
> +#include <sys/conf.h>
>  #include <sys/ctype.h>
>  #include <sys/errno.h>
>  #include <sys/kdb.h>
> _at__at_ -2953,27 +2954,40 _at__at_
>   * Handle loading and unloading for this code.
>   * The only thing we need to link into is the NETISR strucure.
>   */
> +
> +static void
> +ngb_sysinit(void)
> +{
> +	mtx_init(&ng_worklist_mtx, "ng_worklist", NULL, MTX_SPIN);
> +	mtx_init(&ng_typelist_mtx, "netgraph types mutex", NULL, MTX_DEF);
> +	mtx_init(&ng_nodelist_mtx, "netgraph nodelist mutex", NULL, MTX_DEF);
> +	mtx_init(&ng_idhash_mtx, "netgraph idhash mutex", NULL, MTX_DEF);
> +	mtx_init(&ngq_mtx, "netgraph free item list mutex", NULL, MTX_DEF);
> +
> +	/* XXX could use NETISR_MPSAFE but need to verify code */
> +	netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL, 0);
> +}
> +
> +static void
> +ngb_sysuninit(void)
> +{
> +	netisr_unregister(NETISR_NETGRAPH);
> +
> +	mtx_destroy(&ngq_mtx);
> +	mtx_destroy(&ng_idhash_mtx);
> +	mtx_destroy(&ng_nodelist_mtx);
> +	mtx_destroy(&ng_typelist_mtx);
> +	mtx_destroy(&ng_worklist_mtx);
> +}
> +
>  static int
>  ngb_mod_event(module_t mod, int event, void *data)
>  {
> -	int s, error = 0;
> +	int error = 0;
>  
>  	switch (event) {
>  	case MOD_LOAD:
> -		/* Register line discipline */
> -		mtx_init(&ng_worklist_mtx, "ng_worklist", NULL, MTX_SPIN);
> -		mtx_init(&ng_typelist_mtx, "netgraph types mutex", NULL,
> -		    MTX_DEF);
> -		mtx_init(&ng_nodelist_mtx, "netgraph nodelist mutex", NULL,
> -		    MTX_DEF);
> -		mtx_init(&ng_idhash_mtx, "netgraph idhash mutex", NULL,
> -		    MTX_DEF);
> -		mtx_init(&ngq_mtx, "netgraph free item list mutex", NULL,
> -		    MTX_DEF);
> -		s = splimp();
> -		/* XXX could use NETISR_MPSAFE but need to verify code */
> -		netisr_register(NETISR_NETGRAPH, (netisr_t *)ngintr, NULL, 0);
> -		splx(s);
> +		error = 0;
>  		break;
>  	case MOD_UNLOAD:
>  		/* You cant unload it because an interface may be using it.  */
> _at__at_ -2986,12 +3000,9 _at__at_
>  	return (error);
>  }
>  
> -static moduledata_t netgraph_mod = {
> -	"netgraph",
> -	ngb_mod_event,
> -	(NULL)
> -};
> -DECLARE_MODULE(netgraph, netgraph_mod, SI_SUB_DRIVERS, SI_ORDER_MIDDLE);
> +SYSINIT(netgraph, SI_SUB_DRIVERS, SI_ORDER_FIRST, ngb_sysinit, NULL);
> +SYSUNINIT(netgraph, SI_SUB_DRIVERS, SI_ORDER_ANY, ngb_sysuninit, NULL);
> +DEV_MODULE(netgraph, ngb_mod_event, NULL);
>  SYSCTL_NODE(_net, OID_AUTO, graph, CTLFLAG_RW, 0, "netgraph Family");
>  SYSCTL_INT(_net_graph, OID_AUTO, abi_version, CTLFLAG_RD, 0, NG_ABI_VERSION,"");
>  SYSCTL_INT(_net_graph, OID_AUTO, msg_version, CTLFLAG_RD, 0, NG_VERSION, "");
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"