On Thu, Oct 15, 2020 at 10:04 PM Willem de Bruijn
[off-list ref] wrote:
On Tue, Oct 13, 2020 at 12:50 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote:
quoted
On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov [off-list ref] wrote:
quoted
On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
quoted
On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski [off-list ref] wrote:
[...]
quoted
quoted
quoted
quoted
Could you use skb_extensions for this?
Why? If for space, this is already under a non-production ifdef.
I understand, but the skb_ext infra is there for uncommon use cases
like this one. Any particular reason you don't want to use it?
The slight LoC increase?
Is there any precedent for adding the kcov field to other performance
critical structures?
It would be great to come to some conclusion on where exactly to store
kcov_handle. Technically, it is possible to use skb extensions for the
purpose, though it will indeed slightly increase the complexity.
Jakub, you think that kcov_handle should be added as an skb extension,
right?
That'd be preferable. I understand with current use cases it doesn't
really matter, but history shows people come up with all sort of
wonderful use cases down the line. And when they do they rarely go back
and fix such fundamental minutiae.
quoted
Though I do not really object to moving the field, it still seems to
me that sk_buff itself is a better place. Right now skb extensions
store values that are local to specific protocols and that are only
meaningful in the context of these protocols (correct me if I'm
wrong). Although this patch only adds remote kcov coverage to the wifi
code, kcov_handle can be meaningful for other protocols as well - just
like the already existing sk_buff fields. So adding kcov_handle to skb
extensions will break this logical separation.
It's not as much protocols as subsystems. The values are meaningful to
a subsystem which inserts them, that doesn't mean single layer of the
stack. If it was about storing layer's context we would just use
skb->cb.
So I think the kcov use matches pretty well.
skb_extensions was the first thing that came to mind when I read this
patchset too. It is not specific to protocols.
We have long stopped growing sk_buff size.
Thank you all for your comments. I'll use skb extensions in v3 of the series.