Messages in this thread | | | From | Andrey Konovalov <> | Date | Thu, 24 Oct 2019 16:07:05 +0200 | Subject | Re: [PATCH v2 1/3] kcov: remote coverage support |
| |
On Thu, Oct 24, 2019 at 9:27 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Wed, Oct 23, 2019 at 5:24 PM Andrey Konovalov <andreyknvl@google.com> 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.
Will do in v3.
> > ... > > 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.
| |