On Wed, 29 Mar 2023 20:57:26 +0200 Johannes Berg wrote: > On Wed, 2023-03-29 at 11:02 -0700, Jakub Kicinski wrote: > > But it's just a thought, all of the approaches seem acceptable. > > I _think_ I like the one I prototyped this morning better, I'm not sure > I like the subsystem == existing reason part _that_ much. It ultimately > doesn't matter much, it just feels odd that you'd be allowed to have a, > I don't know picking a random example, SKB_DROP_REASON_DUP_FRAG with a > fine-grained higher bits value? > > Not that we'll ever be starved for space ... Ack, for most drop_reasons having higher order bits would make no sense. > > Quick code change perhaps illustrates it best: > > > > Yeah, that ends up really looking very similar :-) > > Then again thinking about the implementation, we'd not be able to use a > simple array for the sub-reasons, or at least that'd waste a bunch of > space, since there are already quite a few 'main' reasons and we'd > want/need to add the mac80211 ones (with sub-reason) at the end. So that > makes a big array for the sub-reasons that's very sparsely populated (*) > Extending with a high 'subsystem' like I did this morning is more > compact here. > > (*) or put the sub-reasons pointer/num with the 'main' reasons into the > drop_reasons[] array but that would take the same additional space Yup, the only difference is that the collector side is simpler if the subsystem is a valid drop reason. For those who don't expect to care about subsystem drop details the aggregate stats are still (bpftrace notation): @stats[reason & 0xffff] = count(); With the higher bits we have to add a layer of stats to the collection? $grp = reason >> 24; if ($grp != 0) @groups[$grp] = count(); else @stats[reason] = count(); That said, I'm probably over-thinking because most will do: @stats[reason] = count(); ... which works the same regardless. > So ... which one do _you_ like better? I think I somewhat prefer the one > with adding a high bits subsystem, but I can relatively easily rejigger > my changes from this morning to implement the semantics you had here > too. No preference. You're coding it up so you're in the best position to pick :)
