On Wed, 29 Mar 2023 10:35:08 +0200 Johannes Berg wrote:
> > Thinking about it again, maybe yours is actually cleaner.
> > Having the subsystem reason on the top bits, I mean.
> > That way after masking the specific bits out the lower bits
> > can still provide a valid "global" drop reason.
>
> Ah, that's not even what I was thinking, but that would work.
>
> What I was thinking was basically the same as you had, just hadn't
> thought about more subsystems yet and was trying to carve out some bits
> for wifi specifically :-)
>
> I don't think we'll really end up with a case where we really want to
> use the low bits for a global reason and a wifi specific reason in
> higher bits together - there are basically no applicable reasons that we
> have today ...
>
> I mean, it basically doesn't make sense to have any of the current
> reasons (such as _IP_CSUM or _IP_INHDR) with a wifi specific reason
> since we wouldn't even go look at the IP header when wifi drops
> something.
>
> The only one that _might_ be relevant would be possibly _PKT_TOO_SMALL
> where wifi has various "reasons" for it to be too small (depending on
> the packet format), but I'm not sure that it would even be helpful to
> have drop monitor report "packet too small" from different layers; today
> it's used from L3/L4, not L2.
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.
But it's just a thought, all of the approaches seem acceptable.
Quick code change perhaps illustrates it best:
diff --git a/include/net/dropreason.h b/include/net/dropreason.h
index c0a3ea806cd5..048402ffa6ad 100644
--- a/include/net/dropreason.h
+++ b/include/net/dropreason.h
@@ -338,11 +338,21 @@ enum skb_drop_reason {
* for another host.
*/
SKB_DROP_REASON_IPV6_NDISC_NS_OTHERHOST,
+ /* packet was unusable? IDK */
+ SKB_DROP_REASON_MAC80211_UNUSABLE,
+ /* also no idea :) */
+ SKB_DROP_REASON_MAC80211_MONITOR,
/**
* @SKB_DROP_REASON_MAX: the maximum of drop reason, which shouldn't be
* used as a real 'reason'
*/
SKB_DROP_REASON_MAX,
+
+ /**
+ * @SKB_DROP_SUBSYS_REASON_MASK: fine grained reason from a particular
+ * subsystem
+ */
+ SKB_DROP_SUBSYS_REASON_MASK = 0xffff0000,
};
#define SKB_DR_INIT(name, reason) \
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 5a782d1d8fd3..a06ba912c793 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -504,6 +504,7 @@ static void net_dm_packet_trace_kfree_skb_hit(void *ignore,
if (!nskb)
return;
+ reason &= ~SKB_DROP_SUBSYS_REASON_MASK;
if (unlikely(reason >= SKB_DROP_REASON_MAX || reason <= 0))
reason = SKB_DROP_REASON_NOT_SPECIFIED;
cb = NET_DM_SKB_CB(nskb);
@@ -611,6 +612,7 @@ static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb,
{
struct net_dm_skb_cb *cb = NET_DM_SKB_CB(skb);
char buf[NET_DM_MAX_SYMBOL_LEN];
+ unsigned int reason, subreason;
struct nlattr *attr;
void *hdr;
int rc;
@@ -627,10 +629,19 @@ static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb,
NET_DM_ATTR_PAD))
goto nla_put_failure;
+ subreason = FIELD_GET(SKB_DROP_SUBSYS_REASON_MASK, cb->reason);
+ reason = cb->reason & ~SKB_DROP_SUBSYS_REASON_MASK;
if (nla_put_string(msg, NET_DM_ATTR_REASON,
- drop_reasons[cb->reason]))
+ drop_reasons[reason]))
goto nla_put_failure;
+ if (subreason) {
+ /* additionally search the per-subsys table,
+ * table is found based on @reason
+ * and indexed with @subreason
+ */
+ }
+
snprintf(buf, sizeof(buf), "%pS", cb->pc);
if (nla_put_string(msg, NET_DM_ATTR_SYMBOL, buf))
goto nla_put_failure;
