| Message ID | af26317c0efd412dd660e81d548a173942f8a0ad.1571333592.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kcov: collect coverage from usb and vhost | expand |
On Thu, Oct 17, 2019 at 07:44:15PM +0200, Andrey Konovalov wrote: > This patch adds kcov_remote_start/kcov_remote_stop annotations to the > vhost_worker function, which is responsible for processing vhost works. > Since vhost_worker is spawned when a vhost device instance is created, > the common kcov handle is used for kcov_remote_start/stop annotations. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > drivers/vhost/vhost.c | 15 +++++++++++++++ > drivers/vhost/vhost.h | 3 +++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 36ca2cf419bf..71a349f6b352 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -357,7 +357,13 @@ static int vhost_worker(void *data) > llist_for_each_entry_safe(work, work_next, node, node) { > clear_bit(VHOST_WORK_QUEUED, &work->flags); > __set_current_state(TASK_RUNNING); > +#ifdef CONFIG_KCOV > + kcov_remote_start(dev->kcov_handle); > +#endif Shouldn't you hide these #ifdefs in a .h file? This is not a "normal" kernel coding style at all. > work->fn(work); > +#ifdef CONFIG_KCOV > + kcov_remote_stop(); > +#endif > if (need_resched()) > schedule(); > } > @@ -546,6 +552,9 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > /* No owner, become one */ > dev->mm = get_task_mm(current); > +#ifdef CONFIG_KCOV > + dev->kcov_handle = current->kcov_handle; > +#endif > worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); > if (IS_ERR(worker)) { > err = PTR_ERR(worker); > @@ -571,6 +580,9 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > if (dev->mm) > mmput(dev->mm); > dev->mm = NULL; > +#ifdef CONFIG_KCOV > + dev->kcov_handle = 0; > +#endif > err_mm: > return err; > } > @@ -682,6 +694,9 @@ void vhost_dev_cleanup(struct vhost_dev *dev) > if (dev->worker) { > kthread_stop(dev->worker); > dev->worker = NULL; > +#ifdef CONFIG_KCOV > + dev->kcov_handle = 0; > +#endif > } > if (dev->mm) > mmput(dev->mm); > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > index e9ed2722b633..010ca1ebcbd5 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -173,6 +173,9 @@ struct vhost_dev { > int iov_limit; > int weight; > int byte_weight; > +#ifdef CONFIG_KCOV > + u64 kcov_handle; > +#endif Why is this a #ifdef at all here? thanks, greg k-h
On Thu, Oct 17, 2019 at 8:18 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Oct 17, 2019 at 07:44:15PM +0200, Andrey Konovalov wrote: > > This patch adds kcov_remote_start/kcov_remote_stop annotations to the > > vhost_worker function, which is responsible for processing vhost works. > > Since vhost_worker is spawned when a vhost device instance is created, > > the common kcov handle is used for kcov_remote_start/stop annotations. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > drivers/vhost/vhost.c | 15 +++++++++++++++ > > drivers/vhost/vhost.h | 3 +++ > > 2 files changed, 18 insertions(+) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index 36ca2cf419bf..71a349f6b352 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -357,7 +357,13 @@ static int vhost_worker(void *data) > > llist_for_each_entry_safe(work, work_next, node, node) { > > clear_bit(VHOST_WORK_QUEUED, &work->flags); > > __set_current_state(TASK_RUNNING); > > +#ifdef CONFIG_KCOV > > + kcov_remote_start(dev->kcov_handle); > > +#endif > > Shouldn't you hide these #ifdefs in a .h file? This is not a "normal" > kernel coding style at all. Well, if it's acceptable to add a kcov_handle field into vhost_dev even when CONFIG_KCOV is not enabled, then we can get rid of those #ifdefs. > > > work->fn(work); > > +#ifdef CONFIG_KCOV > > + kcov_remote_stop(); > > +#endif > > if (need_resched()) > > schedule(); > > } > > @@ -546,6 +552,9 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > > > /* No owner, become one */ > > dev->mm = get_task_mm(current); > > +#ifdef CONFIG_KCOV > > + dev->kcov_handle = current->kcov_handle; > > +#endif > > worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); > > if (IS_ERR(worker)) { > > err = PTR_ERR(worker); > > @@ -571,6 +580,9 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > if (dev->mm) > > mmput(dev->mm); > > dev->mm = NULL; > > +#ifdef CONFIG_KCOV > > + dev->kcov_handle = 0; > > +#endif > > err_mm: > > return err; > > } > > @@ -682,6 +694,9 @@ void vhost_dev_cleanup(struct vhost_dev *dev) > > if (dev->worker) { > > kthread_stop(dev->worker); > > dev->worker = NULL; > > +#ifdef CONFIG_KCOV > > + dev->kcov_handle = 0; > > +#endif > > } > > if (dev->mm) > > mmput(dev->mm); > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > > index e9ed2722b633..010ca1ebcbd5 100644 > > --- a/drivers/vhost/vhost.h > > +++ b/drivers/vhost/vhost.h > > @@ -173,6 +173,9 @@ struct vhost_dev { > > int iov_limit; > > int weight; > > int byte_weight; > > +#ifdef CONFIG_KCOV > > + u64 kcov_handle; > > +#endif > > Why is this a #ifdef at all here? > > thanks, > > greg k-h
On Thu, Oct 17, 2019 at 09:00:18PM +0200, Andrey Konovalov wrote: > On Thu, Oct 17, 2019 at 8:18 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Oct 17, 2019 at 07:44:15PM +0200, Andrey Konovalov wrote: > > > This patch adds kcov_remote_start/kcov_remote_stop annotations to the > > > vhost_worker function, which is responsible for processing vhost works. > > > Since vhost_worker is spawned when a vhost device instance is created, > > > the common kcov handle is used for kcov_remote_start/stop annotations. > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > --- > > > drivers/vhost/vhost.c | 15 +++++++++++++++ > > > drivers/vhost/vhost.h | 3 +++ > > > 2 files changed, 18 insertions(+) > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > index 36ca2cf419bf..71a349f6b352 100644 > > > --- a/drivers/vhost/vhost.c > > > +++ b/drivers/vhost/vhost.c > > > @@ -357,7 +357,13 @@ static int vhost_worker(void *data) > > > llist_for_each_entry_safe(work, work_next, node, node) { > > > clear_bit(VHOST_WORK_QUEUED, &work->flags); > > > __set_current_state(TASK_RUNNING); > > > +#ifdef CONFIG_KCOV > > > + kcov_remote_start(dev->kcov_handle); > > > +#endif > > > > Shouldn't you hide these #ifdefs in a .h file? This is not a "normal" > > kernel coding style at all. > > Well, if it's acceptable to add a kcov_handle field into vhost_dev > even when CONFIG_KCOV is not enabled, then we can get rid of those > #ifdefs. It should be, it's not a big deal and there's not a ton of those structures around that one more field is going to hurt anything... thanks, greg k-h
On Thu, Oct 17, 2019 at 10:28 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Oct 17, 2019 at 09:00:18PM +0200, Andrey Konovalov wrote: > > On Thu, Oct 17, 2019 at 8:18 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, Oct 17, 2019 at 07:44:15PM +0200, Andrey Konovalov wrote: > > > > This patch adds kcov_remote_start/kcov_remote_stop annotations to the > > > > vhost_worker function, which is responsible for processing vhost works. > > > > Since vhost_worker is spawned when a vhost device instance is created, > > > > the common kcov handle is used for kcov_remote_start/stop annotations. > > > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > --- > > > > drivers/vhost/vhost.c | 15 +++++++++++++++ > > > > drivers/vhost/vhost.h | 3 +++ > > > > 2 files changed, 18 insertions(+) > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > index 36ca2cf419bf..71a349f6b352 100644 > > > > --- a/drivers/vhost/vhost.c > > > > +++ b/drivers/vhost/vhost.c > > > > @@ -357,7 +357,13 @@ static int vhost_worker(void *data) > > > > llist_for_each_entry_safe(work, work_next, node, node) { > > > > clear_bit(VHOST_WORK_QUEUED, &work->flags); > > > > __set_current_state(TASK_RUNNING); > > > > +#ifdef CONFIG_KCOV > > > > + kcov_remote_start(dev->kcov_handle); > > > > +#endif > > > > > > Shouldn't you hide these #ifdefs in a .h file? This is not a "normal" > > > kernel coding style at all. > > > > Well, if it's acceptable to add a kcov_handle field into vhost_dev > > even when CONFIG_KCOV is not enabled, then we can get rid of those > > #ifdefs. > > It should be, it's not a big deal and there's not a ton of those > structures around that one more field is going to hurt anything... OK, I'll do that in the next version then, thanks!
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 36ca2cf419bf..71a349f6b352 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -357,7 +357,13 @@ static int vhost_worker(void *data) llist_for_each_entry_safe(work, work_next, node, node) { clear_bit(VHOST_WORK_QUEUED, &work->flags); __set_current_state(TASK_RUNNING); +#ifdef CONFIG_KCOV + kcov_remote_start(dev->kcov_handle); +#endif work->fn(work); +#ifdef CONFIG_KCOV + kcov_remote_stop(); +#endif if (need_resched()) schedule(); } @@ -546,6 +552,9 @@ long vhost_dev_set_owner(struct vhost_dev *dev) /* No owner, become one */ dev->mm = get_task_mm(current); +#ifdef CONFIG_KCOV + dev->kcov_handle = current->kcov_handle; +#endif worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); if (IS_ERR(worker)) { err = PTR_ERR(worker); @@ -571,6 +580,9 @@ long vhost_dev_set_owner(struct vhost_dev *dev) if (dev->mm) mmput(dev->mm); dev->mm = NULL; +#ifdef CONFIG_KCOV + dev->kcov_handle = 0; +#endif err_mm: return err; } @@ -682,6 +694,9 @@ void vhost_dev_cleanup(struct vhost_dev *dev) if (dev->worker) { kthread_stop(dev->worker); dev->worker = NULL; +#ifdef CONFIG_KCOV + dev->kcov_handle = 0; +#endif } if (dev->mm) mmput(dev->mm); diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index e9ed2722b633..010ca1ebcbd5 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -173,6 +173,9 @@ struct vhost_dev { int iov_limit; int weight; int byte_weight; +#ifdef CONFIG_KCOV + u64 kcov_handle; +#endif }; bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len);
This patch adds kcov_remote_start/kcov_remote_stop annotations to the vhost_worker function, which is responsible for processing vhost works. Since vhost_worker is spawned when a vhost device instance is created, the common kcov handle is used for kcov_remote_start/stop annotations. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- drivers/vhost/vhost.c | 15 +++++++++++++++ drivers/vhost/vhost.h | 3 +++ 2 files changed, 18 insertions(+)