netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Aleksandr Nogikh <a.nogikh@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>,
	David Miller <davem@davemloft.net>,
	Johannes Berg <johannes@sipsolutions.net>,
	Eric Dumazet <edumazet@google.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Marco Elver <elver@google.com>,
	LKML <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	Aleksandr Nogikh <nogikh@google.com>,
	Florian Westphal <fw@strlen.de>
Subject: Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff
Date: Tue, 13 Oct 2020 09:50:38 -0700	[thread overview]
Message-ID: <20201013095038.61ba8f55@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <CADpXja8i4YPT=vcuCr412RYqRMjTOGuaMW2dyV0j7BtEwNBgFA@mail.gmail.com>

On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote:
> On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > >
> > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:  
> > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:  
> [...]
> > > > > 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.

> 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. 

Florian, what's your take?

  reply	other threads:[~2020-10-13 16:50 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-07 10:17 [PATCH 0/2] net, mac80211: enable KCOV remote coverage collection for 802.11 frame handling Aleksandr Nogikh
2020-10-07 10:17 ` [PATCH 1/2] net: store KCOV remote handle in sk_buff Aleksandr Nogikh
2020-10-09 23:15   ` Jakub Kicinski
2020-10-10  7:54     ` Dmitry Vyukov
2020-10-10 15:14       ` Jakub Kicinski
2020-10-12  6:04         ` Dmitry Vyukov
2020-10-13 15:59           ` Aleksandr Nogikh
2020-10-13 16:50             ` Jakub Kicinski [this message]
2020-10-15 19:04               ` Willem de Bruijn
2020-10-16 14:20                 ` Aleksandr Nogikh
2020-10-12  7:12   ` Johannes Berg
2020-10-12 10:10     ` Aleksandr Nogikh
2020-10-07 10:17 ` [PATCH 2/2] mac80211: add KCOV remote annotations to incoming frame processing Aleksandr Nogikh
2020-10-07 11:48 ` [PATCH 0/2] net, mac80211: enable KCOV remote coverage collection for 802.11 frame handling Johannes Berg
2020-10-07 14:40   ` Aleksandr Nogikh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201013095038.61ba8f55@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=a.nogikh@gmail.com \
    --cc=andreyknvl@google.com \
    --cc=davem@davemloft.net \
    --cc=dvyukov@google.com \
    --cc=edumazet@google.com \
    --cc=elver@google.com \
    --cc=fw@strlen.de \
    --cc=johannes@sipsolutions.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nogikh@google.com \
    --subject='Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox