On Wed, 2023-03-29 at 11:02 -0700, Jakub Kicinski wrote: > > No, no what I was trying to say is that instead of using the upper bits > to identify the space (with 0 being the current enum skb_drop_reason) > we could use entries in enum skb_drop_reason. In hope that it'd make > the fine grained subsystem reason seem more like additional information > than a completely parallel system. Ah! Looking at your code example ... right, so you'd see "mac80211 drop unusable" or "mac80211 drop to monitor", and fine-grained in the higher bits. > 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 ... > 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 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. Anyone else have an opinion? :) johannes
