netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Aleksandr Nogikh <a.nogikh@gmail.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: davem@davemloft.net, kuba@kernel.org,
	Eric Dumazet <edumazet@google.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Marco Elver <elver@google.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	linux-wireless@vger.kernel.org, nogikh@google.com
Subject: Re: [PATCH 0/2] net, mac80211: enable KCOV remote coverage collection for 802.11 frame handling
Date: Wed, 7 Oct 2020 17:40:52 +0300	[thread overview]
Message-ID: <CADpXja8oh2nHpNFjQ-+iDUebPx7R_HcOuoakzmKvHp=kuj8rpA@mail.gmail.com> (raw)
In-Reply-To: <bec6415925c213a2e3eb86e80d6982b82180f019.camel@sipsolutions.net>

On Wed, 7 Oct 2020 at 14:48, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote:
[...]
> > Aleksandr Nogikh (2):
> >   net: store KCOV remote handle in sk_buff
>
> Can you explain that a bit better? What is a "remote handle"? What does
> it do in the SKB?
>
> I guess I'd have to know more about "kcov_common_handle()" to understand
> this bit.

Normally, KCOV collects coverage information for the code that is
executed inside the system call context. It is easy to identify where
that coverage should go and whether it should be collected at all by
looking at the current process. If KCOV was enabled on that process,
coverage will be stored in a buffer specific to that process.
Howerever, it is not always enough as some handling can happen
elsewhere (e.g. in separate kernel threads).

That is why remote KOV coverage collection was introduced. When it is
impossible to infer KCOV-related info just by looking at the currently
running process, we need to manually pass some information to the code
that is of interest to us.  The information takes the form of 64 bit
integers (remote handles). Zero is the special value that corresponds
to an empty handle. More details on KCOV and remote coverage
collection can be found here: Documentation/dev-tools/kcov.rst.

In this patch, we obtain the remote handle from KCOV (in this case by
executing kcov_common_handle()) and attach it to newly allocated
SKBs. If we're in a system call context, the SKB will be tied to the
process that issued the syscall (if that process is interested in
remote coverage collection). So when
kcov_remote_start_common(skb_get_kcov_handle(skb)) is executed, it is
possible to determine whether coverage is required and where it should
be stored.

I have just realized that the default kcov_handle initialization as it
was implemented in this patch is not really robust. If an skb is
allocated during a hard IRQ, kcov_common_handle() will return a remote
handle for the interrupted thread instead of returning 0, and that is
not desirable since it will occasionally lead to wrong kcov_handles. I
will fix it in the next version of the patch.

> >   mac80211: add KCOV remote annotations to incoming frame processing
>
> This seems fine, but a bit too limited? You tagged
> only ieee80211_tasklet_handler() which calls ieee80211_rx()
> or ieee80211_tx_status(), but
>
> 1) I'm not even sure ieee80211_tx_status() counts (it's processing
> locally generated frames after they round-tripped into the driver
> (although in mesh it could be remote originated but retransmitted
> frames, so I guess it makes some sense?); and
>
> 2) there are many other ways that ieee80211_rx() could get called.
>
> It seems to me it'd make more sense to (also) annotate ieee80211_rx()
> itself?

Yes, it definitely makes more sense to annotate ieee80211_rx()
directly. Collecting coverage for ieee80211_tx_status() does not seem
to be needed now and can be added later if there's a use case for it.

Thank you for the suggestion. I will implement it in the second
version of the patch.

--
Best regards,
Aleksandr

      reply	other threads:[~2020-10-07 14:41 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-07 10:17 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
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 [this message]

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='CADpXja8oh2nHpNFjQ-+iDUebPx7R_HcOuoakzmKvHp=kuj8rpA@mail.gmail.com' \
    --to=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=johannes@sipsolutions.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nogikh@google.com \
    --subject='Re: [PATCH 0/2] net, mac80211: enable KCOV remote coverage collection for 802.11 frame handling' \
    /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