Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Mar 2023 12:09:59 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     Eric Dumazet <edumazet@google.com>, netdev@vger.kernel.org,
        linux-wireless@vger.kernel.org
Subject: Re: traceability of wifi packet drops
Message-ID: <20230329120959.5f9eef1c@kernel.org>
In-Reply-To: <37311ab0f31d719a65858de31cec7a840cf8fe40.camel@sipsolutions.net>
References: <00659771ed54353f92027702c5bbb84702da62ce.camel@sipsolutions.net>
        <20230327180950.79e064da@kernel.org>
        <abcf4b9aed8adad05841234dad103ced15f9bfb2.camel@sipsolutions.net>
        <20230328155826.38e9e077@kernel.org>
        <8304ec7e430815edf3b79141c90272e36683e085.camel@sipsolutions.net>
        <20230329110205.1202eb60@kernel.org>
        <37311ab0f31d719a65858de31cec7a840cf8fe40.camel@sipsolutions.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 :)