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.

Will do in v3.

...

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.

For remote kcov instances kcov->t points to the thread that created
the kcov device (I'll update the comment in struct kcov). When a task
is between kcov_remote_start() and kcov_remote_stop(), it's t->kcov
point to the remote kcov. So t is current for this task, and
t->kcov->t is the task that created the kcov instance. I'll expand the
comment to explain this better.

Otherwise the series look good to me:

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

Great, thanks!

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.