Re: [PATCH v2 1/2] net: extend drop reasons for multiple subsystems

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 30 Mar 2023 23:22:26 +0200 Johannes Berg wrote:
> diff --git a/include/net/dropreason.h b/include/net/dropreason.h
> index c0a3ea806cd5..d7a134c108ad 100644
> --- a/include/net/dropreason.h
> +++ b/include/net/dropreason.h
> @@ -339,10 +339,28 @@ enum skb_drop_reason {
>  	 */
>  	SKB_DROP_REASON_IPV6_NDISC_NS_OTHERHOST,
>  	/**
> -	 * @SKB_DROP_REASON_MAX: the maximum of drop reason, which shouldn't be
> -	 * used as a real 'reason'
> -	 */
> +	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
> +	 * shouldn't be used as a real 'reason' - only for tracing code gen
> +         */
>  	SKB_DROP_REASON_MAX,
> +
> +	/** @SKB_DROP_REASON_SUBSYS_MASK: subsystem mask in drop reasons,
> +	 * see &enum skb_drop_reason_subsys
> +	 */
> +	SKB_DROP_REASON_SUBSYS_MASK = 0xffff0000,
> +};
> +
> +#define SKB_DROP_REASON_SUBSYS_SHIFT	16
> +
> +/**
> + * enum skb_drop_reason_subsys - subsystem tag for (extended) drop reasons
> + */
> +enum skb_drop_reason_subsys {
> +	/** @SKB_DROP_REASON_SUBSYS_CORE: core drop reasons defined above */
> +	SKB_DROP_REASON_SUBSYS_CORE,
> +
> +	/** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */
> +	SKB_DROP_REASON_SUBSYS_NUM
>  };
>  
>  #define SKB_DR_INIT(name, reason)				\
> @@ -358,6 +376,17 @@ enum skb_drop_reason {
>  			SKB_DR_SET(name, reason);		\
>  	} while (0)
>  
> -extern const char * const drop_reasons[];
> +struct drop_reason_list {
> +	const char * const *reasons;
> +	size_t n_reasons;
> +};
> +
> +/* Note: due to dynamic registrations, access must be under RCU */
> +extern const struct drop_reason_list __rcu *
> +drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM];
> +
> +void drop_reasons_register_subsys(enum skb_drop_reason_subsys subsys,
> +				  const struct drop_reason_list *list);
> +void drop_reasons_unregister_subsys(enum skb_drop_reason_subsys subsys);

dropreason.h is included by skbuff.h because history, but I don't think
any of the new stuff must be visible in skbuff.h.

Could you make a new header, and put as much of this stuff there as
possible? Our future selves will thank us for shorter rebuild times..

>  #undef FN
>  #define FN(reason) [SKB_DROP_REASON_##reason] = #reason,
> -const char * const drop_reasons[] = {
> +static const char * const drop_reasons[] = {
>  	[SKB_CONSUMED] = "CONSUMED",
>  	DEFINE_DROP_REASON(FN, FN)
>  };
> -EXPORT_SYMBOL(drop_reasons);
> +
> +static const struct drop_reason_list drop_reasons_core = {
> +	.reasons = drop_reasons,
> +	.n_reasons = ARRAY_SIZE(drop_reasons),
> +};
> +
> +const struct drop_reason_list __rcu *
> +drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM] = {
> +	[SKB_DROP_REASON_SUBSYS_CORE] = &drop_reasons_core,
> +};
> +EXPORT_SYMBOL(drop_reasons_by_subsys);
> +
> +/**
> + * drop_reasons_register_subsys - register another drop reason subsystem
> + * @subsys: the subsystem to register, must not be the core
> + * @list: the list of drop reasons within the subsystem, must point to
> + *	a statically initialized list
> + */
> +void drop_reasons_register_subsys(enum skb_drop_reason_subsys subsys,
> +				  const struct drop_reason_list *list)
> +{
> +	if (WARN(subsys <= SKB_DROP_REASON_SUBSYS_CORE ||
> +		 subsys >= ARRAY_SIZE(drop_reasons_by_subsys),
> +		 "invalid subsystem %d\n", subsys))
> +		return;
> +
> +	/* must point to statically allocated memory, so INIT is OK */
> +	RCU_INIT_POINTER(drop_reasons_by_subsys[subsys], list);
> +}
> +EXPORT_SYMBOL_GPL(drop_reasons_register_subsys);
> +
> +/**
> + * drop_reasons_unregister_subsys - unregister a drop reason subsystem
> + * @subsys: the subsystem to remove, must not be the core
> + *
> + * Note: This will synchronize_rcu() to ensure no users when it returns.
> + */
> +void drop_reasons_unregister_subsys(enum skb_drop_reason_subsys subsys)
> +{
> +	if (WARN(subsys <= SKB_DROP_REASON_SUBSYS_CORE ||
> +		 subsys >= ARRAY_SIZE(drop_reasons_by_subsys),
> +		 "invalid subsystem %d\n", subsys))
> +		return;
> +
> +	RCU_INIT_POINTER(drop_reasons_by_subsys[subsys], NULL);
> +
> +	synchronize_rcu();
> +}
> +EXPORT_SYMBOL_GPL(drop_reasons_unregister_subsys);

Weak preference to also take the code out of skbuff.c but that's not as
important.


You To'd both wireless and netdev, who are you expecting to apply this?
:S