Message ID | 20230330212227.928595-1-johannes@sipsolutions.net (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [v2,1/2] net: extend drop reasons for multiple subsystems | expand |
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
Hi Johannes, kernel test robot noticed the following build warnings: [auto build test WARNING on wireless-next/main] [also build test WARNING on wireless/main horms-ipvs/master net/main net-next/main linus/master v6.3-rc6 next-20230413] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Berg/mac80211-use-the-new-drop-reasons-infrastructure/20230331-052445 base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main patch link: https://lore.kernel.org/r/20230330212227.928595-1-johannes%40sipsolutions.net patch subject: [PATCH v2 1/2] net: extend drop reasons for multiple subsystems config: m68k-randconfig-s042-20230413 (https://download.01.org/0day-ci/archive/20230414/202304141104.ixaAlfxh-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/64925179be74db98280d706236e37e05bf7b5cca git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Johannes-Berg/mac80211-use-the-new-drop-reasons-infrastructure/20230331-052445 git checkout 64925179be74db98280d706236e37e05bf7b5cca # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k SHELL=/bin/bash net/core/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202304141104.ixaAlfxh-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> net/core/skbuff.c:138:42: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected struct drop_reason_list const [noderef] __rcu * @@ got struct drop_reason_list const * @@ net/core/skbuff.c:138:42: sparse: expected struct drop_reason_list const [noderef] __rcu * net/core/skbuff.c:138:42: sparse: got struct drop_reason_list const * vim +138 net/core/skbuff.c 135 136 const struct drop_reason_list __rcu * 137 drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM] = { > 138 [SKB_DROP_REASON_SUBSYS_CORE] = &drop_reasons_core, 139 }; 140 EXPORT_SYMBOL(drop_reasons_by_subsys); 141
On Fri, 2023-03-31 at 21:36 -0700, Jakub Kicinski wrote: > > > +/* 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.. Sure. Not sure it'll make a big difference in rebuild, but we'll see :) I ended up moving dropreason.h to dropreason-core.h first, that way we also have a naming scheme for non-core dropreason files should they become visible outside of the subsystem (i.e. mac80211 just has them internally). Dunno, let me know if you prefer something else, I just couldn't come up with a non-confusing longer name for the new thing. > Weak preference to also take the code out of skbuff.c but that's not as > important. I guess I can create a new dropreason.c, but is that worth it? It's only a few lines. Let me know, then I can resend. > You To'd both wireless and netdev, who are you expecting to apply this? > :S Good question :) The first patch (patches in v3) really should go through net-next I suppose, and I wouldn't mind if the other one did as well, it doesn't right now touch anything likely to change. johannes
On Fri, 14 Apr 2023 11:25:08 +0200 Johannes Berg wrote: > On Fri, 2023-03-31 at 21:36 -0700, Jakub Kicinski wrote: > > > > > +/* 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.. > > Sure. Not sure it'll make a big difference in rebuild, but we'll see :) > > I ended up moving dropreason.h to dropreason-core.h first, that way we > also have a naming scheme for non-core dropreason files should they > become visible outside of the subsystem (i.e. mac80211 just has them > internally). > > Dunno, let me know if you prefer something else, I just couldn't come up > with a non-confusing longer name for the new thing. Sounds good. > > Weak preference to also take the code out of skbuff.c but that's not as > > important. > > I guess I can create a new dropreason.c, but is that worth it? It's only > a few lines. Let me know, then I can resend. It's hard to tell. Most additions to the core are small at the start so we end up chucking all of them into a handful of existing source files. And those files grow and grow. Splitting the later is extra work and makes backports harder. It's a game of predicting which code will likely grow into a reasonable ~500+ LoC at some point, and which code will not. I have the feeling that dropreason code will grow. But yes, it's still fairly small, we can defer. > > You To'd both wireless and netdev, who are you expecting to apply this? > > :S > > Good question :) > > The first patch (patches in v3) really should go through net-next I > suppose, and I wouldn't mind if the other one did as well, it doesn't > right now touch anything likely to change. SG!
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); #endif diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index 5a782d1d8fd3..c6c60dc75b2d 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -21,6 +21,7 @@ #include <linux/workqueue.h> #include <linux/netlink.h> #include <linux/net_dropmon.h> +#include <linux/bitfield.h> #include <linux/percpu.h> #include <linux/timer.h> #include <linux/bitops.h> @@ -504,8 +505,6 @@ static void net_dm_packet_trace_kfree_skb_hit(void *ignore, if (!nskb) return; - if (unlikely(reason >= SKB_DROP_REASON_MAX || reason <= 0)) - reason = SKB_DROP_REASON_NOT_SPECIFIED; cb = NET_DM_SKB_CB(nskb); cb->reason = reason; cb->pc = location; @@ -552,9 +551,9 @@ static size_t net_dm_in_port_size(void) } #define NET_DM_MAX_SYMBOL_LEN 40 +#define NET_DM_MAX_REASON_LEN 50 -static size_t net_dm_packet_report_size(size_t payload_len, - enum skb_drop_reason reason) +static size_t net_dm_packet_report_size(size_t payload_len) { size_t size; @@ -576,7 +575,7 @@ static size_t net_dm_packet_report_size(size_t payload_len, /* NET_DM_ATTR_PROTO */ nla_total_size(sizeof(u16)) + /* NET_DM_ATTR_REASON */ - nla_total_size(strlen(drop_reasons[reason]) + 1) + + nla_total_size(NET_DM_MAX_REASON_LEN + 1) + /* NET_DM_ATTR_PAYLOAD */ nla_total_size(payload_len); } @@ -610,6 +609,8 @@ static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb, size_t payload_len) { struct net_dm_skb_cb *cb = NET_DM_SKB_CB(skb); + const struct drop_reason_list *list = NULL; + unsigned int subsys, subsys_reason; char buf[NET_DM_MAX_SYMBOL_LEN]; struct nlattr *attr; void *hdr; @@ -627,9 +628,24 @@ static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb, NET_DM_ATTR_PAD)) goto nla_put_failure; + rcu_read_lock(); + subsys = u32_get_bits(cb->reason, SKB_DROP_REASON_SUBSYS_MASK); + if (subsys < SKB_DROP_REASON_SUBSYS_NUM) + list = rcu_dereference(drop_reasons_by_subsys[subsys]); + subsys_reason = cb->reason & ~SKB_DROP_REASON_SUBSYS_MASK; + if (!list || + subsys_reason >= list->n_reasons || + !list->reasons[subsys_reason] || + strlen(list->reasons[subsys_reason]) > NET_DM_MAX_REASON_LEN) { + list = rcu_dereference(drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_CORE]); + subsys_reason = SKB_DROP_REASON_NOT_SPECIFIED; + } if (nla_put_string(msg, NET_DM_ATTR_REASON, - drop_reasons[cb->reason])) + list->reasons[subsys_reason])) { + rcu_read_unlock(); goto nla_put_failure; + } + rcu_read_unlock(); snprintf(buf, sizeof(buf), "%pS", cb->pc); if (nla_put_string(msg, NET_DM_ATTR_SYMBOL, buf)) @@ -687,9 +703,7 @@ static void net_dm_packet_report(struct sk_buff *skb) if (net_dm_trunc_len) payload_len = min_t(size_t, net_dm_trunc_len, payload_len); - msg = nlmsg_new(net_dm_packet_report_size(payload_len, - NET_DM_SKB_CB(skb)->reason), - GFP_KERNEL); + msg = nlmsg_new(net_dm_packet_report_size(payload_len), GFP_KERNEL); if (!msg) goto out; diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 050a875d09c5..d4b2a5cedbd4 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -58,6 +58,7 @@ #include <linux/scatterlist.h> #include <linux/errqueue.h> #include <linux/prefetch.h> +#include <linux/bitfield.h> #include <linux/if_vlan.h> #include <linux/mpls.h> #include <linux/kcov.h> @@ -122,11 +123,59 @@ EXPORT_SYMBOL(sysctl_max_skb_frags); #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); /** * skb_panic - private function for out-of-line support @@ -984,7 +1033,10 @@ bool __kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason) if (unlikely(!skb_unref(skb))) return false; - DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX); + DEBUG_NET_WARN_ON_ONCE(reason == SKB_NOT_DROPPED_YET || + u32_get_bits(reason, + SKB_DROP_REASON_SUBSYS_MASK) >= + SKB_DROP_REASON_SUBSYS_NUM); if (reason == SKB_CONSUMED) trace_consume_skb(skb, __builtin_return_address(0));