On Thu, Oct 24, 2019 at 9:27 AM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>
> On Wed, Oct 23, 2019 at 5:24 PM Andrey Konovalov <andreyknvl@xxxxxxxxxx> wrote:
> >
> > This patch adds background thread coverage collection ability to kcov.
> ...
> > +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().
>
> ...
> > 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@xxxxxxxxxx>
>
> 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.
