On Thu, Oct 24, 2019 at 9:27 AM Dmitry Vyukov [off-list ref] wrote:

On Wed, Oct 23, 2019 at 5:24 PM Andrey Konovalov [off-list ref] wrote:

quoted

This patch adds background thread coverage collection ability to kcov.

...

quoted

+static struct kcov_remote *kcov_remote_add(struct kcov *kcov, u64 handle)
+{
+       struct kcov_remote *remote;
+
+       if (kcov_remote_find(handle))
+               return ERR_PTR(-EEXIST);
+       remote = kmalloc(sizeof(*remote), GFP_ATOMIC);
+       if (!remote)
+               return ERR_PTR(-ENOMEM);
+       remote->handle = handle;
+       remote->kcov = kcov;
+       hash_add(kcov_remote_map, &remote->hnode, handle);

I think it will make sense to check that there is no existing kcov
with the same handle registered. Such condition will be extremely hard
to debug based on episodically missing coverage.

Although looking at this again: we already check that by calling
kcov_remote_find().

...

quoted

 void kcov_task_exit(struct task_struct *t)
 {
        struct kcov *kcov;

@@ -256,15 +401,23 @@ void kcov_task_exit(struct task_struct *t)
        kcov = t->kcov;
        if (kcov == NULL)
                return;
+
        spin_lock(&kcov->lock);
+       kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t);
+       /*
+        * If !kcov->remote, this checks that t->kcov->t == t.
+        * If kcov->remote == true then the exiting task is either:
+        * 1. a remote task between kcov_remote_start() and kcov_remote_stop(),
+        *    in this case t != kcov->t and we'll print a warning; or

Why? Is kcov->t == NULL for remote kcov's? May be worth mentioning in
the comment b/c it's a very condensed form to check lots of different
things at once.

Otherwise the series look good to me:

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

But Andrew's comments stand. It's possible I understand all of this
only because I already know how it works and why it works this way.