| Message ID | c07a2deae7a75e394de272c1a33cfcc1f667af92.1602522185.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] kcov, usbip: collect coverage from vhci_rx_loop | expand |
On 10/12/20 11:10 AM, Andrey Konovalov wrote: > From: Nazime Hande Harputluoglu <handeharputlu@google.com> > > Add kcov_remote_start()/kcov_remote_stop() annotations to the > vhci_rx_loop() function, which is responsible for parsing USB/IP packets > coming into USB/IP client. > > Since vhci_rx_loop() threads are spawned per vhci_hcd device instance, the > common kcov handle is used for kcov_remote_start()/stop() annotations > (see Documentation/dev-tools/kcov.rst for details). As the result kcov > can now be used to collect coverage from vhci_rx_loop() threads. > > Signed-off-by: Nazime Hande Harputluoglu <handeharputlu@google.com> > --- > > Changes v1->v2: > - Fix spacing issues. > - Add ifdef CONFIG_KCOV around kcov_handle in usbip_device struct. > Does this compile without CONFIG_KCOV? > --- > drivers/usb/usbip/usbip_common.h | 4 ++++ > drivers/usb/usbip/vhci_rx.c | 3 +++ > drivers/usb/usbip/vhci_sysfs.c | 12 +++++++----- > 3 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h > index 8be857a4fa13..0906182011d6 100644 > --- a/drivers/usb/usbip/usbip_common.h > +++ b/drivers/usb/usbip/usbip_common.h > @@ -277,6 +277,10 @@ struct usbip_device { > void (*reset)(struct usbip_device *); > void (*unusable)(struct usbip_device *); > } eh_ops; > + > +#ifdef CONFIG_KCOV > + u64 kcov_handle; > +#endif > }; > > #define kthread_get_run(threadfn, data, namefmt, ...) \ > diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c > index 266024cbb64f..473f14587bd5 100644 > --- a/drivers/usb/usbip/vhci_rx.c > +++ b/drivers/usb/usbip/vhci_rx.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2003-2008 Takahiro Hirofuchi > */ > > +#include <linux/kcov.h> > #include <linux/kthread.h> > #include <linux/slab.h> > > @@ -261,7 +262,9 @@ int vhci_rx_loop(void *data) > if (usbip_event_happened(ud)) > break; > > + kcov_remote_start_common(ud->kcov_handle); You are referencing kcov_handle defined in CONFIG_KCOV scope here. Does this compile for you without CONFIG_KCOV? > vhci_rx_pdu(ud); > + kcov_remote_stop(); > } > > return 0; > diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c > index be37aec250c2..966f1f5cafb1 100644 > --- a/drivers/usb/usbip/vhci_sysfs.c > +++ b/drivers/usb/usbip/vhci_sysfs.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2015-2016 Nobuo Iwata > */ > > +#include <linux/kcov.h> > #include <linux/kthread.h> > #include <linux/file.h> > #include <linux/net.h> > @@ -378,11 +379,12 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, > dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n", > devid, speed, usb_speed_string(speed)); > > - vdev->devid = devid; > - vdev->speed = speed; > - vdev->ud.sockfd = sockfd; > - vdev->ud.tcp_socket = socket; > - vdev->ud.status = VDEV_ST_NOTASSIGNED; > + vdev->devid = devid; > + vdev->speed = speed; > + vdev->ud.sockfd = sockfd; > + vdev->ud.tcp_socket = socket; > + vdev->ud.status = VDEV_ST_NOTASSIGNED; > + vdev->ud.kcov_handle = kcov_common_handle(); Don't change spacing for other variables. Add just the new code. Don't you need CONFIG_KCOV around this new code? > > spin_unlock(&vdev->ud.lock); > spin_unlock_irqrestore(&vhci->lock, flags); > thanks, -- Shuah
On Tue, Oct 13, 2020 at 7:28 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 10/12/20 11:10 AM, Andrey Konovalov wrote: > > From: Nazime Hande Harputluoglu <handeharputlu@google.com> > > > > Add kcov_remote_start()/kcov_remote_stop() annotations to the > > vhci_rx_loop() function, which is responsible for parsing USB/IP packets > > coming into USB/IP client. > > > > Since vhci_rx_loop() threads are spawned per vhci_hcd device instance, the > > common kcov handle is used for kcov_remote_start()/stop() annotations > > (see Documentation/dev-tools/kcov.rst for details). As the result kcov > > can now be used to collect coverage from vhci_rx_loop() threads. > > > > Signed-off-by: Nazime Hande Harputluoglu <handeharputlu@google.com> > > --- > > > > Changes v1->v2: > > - Fix spacing issues. > > - Add ifdef CONFIG_KCOV around kcov_handle in usbip_device struct. > > > > Does this compile without CONFIG_KCOV? > > > --- > > drivers/usb/usbip/usbip_common.h | 4 ++++ > > drivers/usb/usbip/vhci_rx.c | 3 +++ > > drivers/usb/usbip/vhci_sysfs.c | 12 +++++++----- > > 3 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h > > index 8be857a4fa13..0906182011d6 100644 > > --- a/drivers/usb/usbip/usbip_common.h > > +++ b/drivers/usb/usbip/usbip_common.h > > @@ -277,6 +277,10 @@ struct usbip_device { > > void (*reset)(struct usbip_device *); > > void (*unusable)(struct usbip_device *); > > } eh_ops; > > + > > +#ifdef CONFIG_KCOV > > + u64 kcov_handle; > > +#endif > > }; > > > > #define kthread_get_run(threadfn, data, namefmt, ...) \ > > diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c > > index 266024cbb64f..473f14587bd5 100644 > > --- a/drivers/usb/usbip/vhci_rx.c > > +++ b/drivers/usb/usbip/vhci_rx.c > > @@ -3,6 +3,7 @@ > > * Copyright (C) 2003-2008 Takahiro Hirofuchi > > */ > > > > +#include <linux/kcov.h> > > #include <linux/kthread.h> > > #include <linux/slab.h> > > > > @@ -261,7 +262,9 @@ int vhci_rx_loop(void *data) > > if (usbip_event_happened(ud)) > > break; > > > > + kcov_remote_start_common(ud->kcov_handle); > > You are referencing kcov_handle defined in CONFIG_KCOV scope > here. Does this compile for you without CONFIG_KCOV? > > > vhci_rx_pdu(ud); > > + kcov_remote_stop(); > > } > > > > return 0; > > diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c > > index be37aec250c2..966f1f5cafb1 100644 > > --- a/drivers/usb/usbip/vhci_sysfs.c > > +++ b/drivers/usb/usbip/vhci_sysfs.c > > @@ -4,6 +4,7 @@ > > * Copyright (C) 2015-2016 Nobuo Iwata > > */ > > > > +#include <linux/kcov.h> > > #include <linux/kthread.h> > > #include <linux/file.h> > > #include <linux/net.h> > > @@ -378,11 +379,12 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, > > dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n", > > devid, speed, usb_speed_string(speed)); > > > > - vdev->devid = devid; > > - vdev->speed = speed; > > - vdev->ud.sockfd = sockfd; > > - vdev->ud.tcp_socket = socket; > > - vdev->ud.status = VDEV_ST_NOTASSIGNED; > > + vdev->devid = devid; > > + vdev->speed = speed; > > + vdev->ud.sockfd = sockfd; > > + vdev->ud.tcp_socket = socket; > > + vdev->ud.status = VDEV_ST_NOTASSIGNED; > > + vdev->ud.kcov_handle = kcov_common_handle(); > > Don't change spacing for other variables. Add just the new > code. Don't you need CONFIG_KCOV around this new code? Yeah, this breaks without CONFIG_KCOV, my bad. I'll fix everything and submit v3. Thank you!
On Tue, Oct 13, 2020 at 7:28 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 10/12/20 11:10 AM, Andrey Konovalov wrote: > > From: Nazime Hande Harputluoglu <handeharputlu@google.com> > > > > Add kcov_remote_start()/kcov_remote_stop() annotations to the > > vhci_rx_loop() function, which is responsible for parsing USB/IP packets > > coming into USB/IP client. > > > > Since vhci_rx_loop() threads are spawned per vhci_hcd device instance, the > > common kcov handle is used for kcov_remote_start()/stop() annotations > > (see Documentation/dev-tools/kcov.rst for details). As the result kcov > > can now be used to collect coverage from vhci_rx_loop() threads. > > > > Signed-off-by: Nazime Hande Harputluoglu <handeharputlu@google.com> > > --- > > > > Changes v1->v2: > > - Fix spacing issues. > > - Add ifdef CONFIG_KCOV around kcov_handle in usbip_device struct. > > > > Does this compile without CONFIG_KCOV? > > > --- > > drivers/usb/usbip/usbip_common.h | 4 ++++ > > drivers/usb/usbip/vhci_rx.c | 3 +++ > > drivers/usb/usbip/vhci_sysfs.c | 12 +++++++----- > > 3 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h > > index 8be857a4fa13..0906182011d6 100644 > > --- a/drivers/usb/usbip/usbip_common.h > > +++ b/drivers/usb/usbip/usbip_common.h > > @@ -277,6 +277,10 @@ struct usbip_device { > > void (*reset)(struct usbip_device *); > > void (*unusable)(struct usbip_device *); > > } eh_ops; > > + > > +#ifdef CONFIG_KCOV > > + u64 kcov_handle; > > +#endif Hi Shuah, We could have this field always defined, which allows us to not check CONFIG_KCOV in the places where it's used (this is what we do for vhost; the kcov functions will be optimized away). Or we could keep the ifdef CONFIG_KCOV check here, and then add the same checks to other places. What would be your preference here? Thanks!
On 10/16/20 8:18 AM, Andrey Konovalov wrote: > On Tue, Oct 13, 2020 at 7:28 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >> >> On 10/12/20 11:10 AM, Andrey Konovalov wrote: >>> From: Nazime Hande Harputluoglu <handeharputlu@google.com> >>> >>> Add kcov_remote_start()/kcov_remote_stop() annotations to the >>> vhci_rx_loop() function, which is responsible for parsing USB/IP packets >>> coming into USB/IP client. >>> >>> Since vhci_rx_loop() threads are spawned per vhci_hcd device instance, the >>> common kcov handle is used for kcov_remote_start()/stop() annotations >>> (see Documentation/dev-tools/kcov.rst for details). As the result kcov >>> can now be used to collect coverage from vhci_rx_loop() threads. >>> >>> Signed-off-by: Nazime Hande Harputluoglu <handeharputlu@google.com> >>> --- >>> >>> Changes v1->v2: >>> - Fix spacing issues. >>> - Add ifdef CONFIG_KCOV around kcov_handle in usbip_device struct. >>> >> >> Does this compile without CONFIG_KCOV? >> >>> --- >>> drivers/usb/usbip/usbip_common.h | 4 ++++ >>> drivers/usb/usbip/vhci_rx.c | 3 +++ >>> drivers/usb/usbip/vhci_sysfs.c | 12 +++++++----- >>> 3 files changed, 14 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h >>> index 8be857a4fa13..0906182011d6 100644 >>> --- a/drivers/usb/usbip/usbip_common.h >>> +++ b/drivers/usb/usbip/usbip_common.h >>> @@ -277,6 +277,10 @@ struct usbip_device { >>> void (*reset)(struct usbip_device *); >>> void (*unusable)(struct usbip_device *); >>> } eh_ops; >>> + >>> +#ifdef CONFIG_KCOV >>> + u64 kcov_handle; >>> +#endif > > Hi Shuah, > > We could have this field always defined, which allows us to not check > CONFIG_KCOV in the places where it's used (this is what we do for > vhost; the kcov functions will be optimized away). Or we could keep > the ifdef CONFIG_KCOV check here, and then add the same checks to > other places. > > What would be your preference here? Let's keep it in ifdef CONFIG_KCOV and add checks to keep this limited to just the kcov case. thanks, -- Shuah
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h index 8be857a4fa13..0906182011d6 100644 --- a/drivers/usb/usbip/usbip_common.h +++ b/drivers/usb/usbip/usbip_common.h @@ -277,6 +277,10 @@ struct usbip_device { void (*reset)(struct usbip_device *); void (*unusable)(struct usbip_device *); } eh_ops; + +#ifdef CONFIG_KCOV + u64 kcov_handle; +#endif }; #define kthread_get_run(threadfn, data, namefmt, ...) \ diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c index 266024cbb64f..473f14587bd5 100644 --- a/drivers/usb/usbip/vhci_rx.c +++ b/drivers/usb/usbip/vhci_rx.c @@ -3,6 +3,7 @@ * Copyright (C) 2003-2008 Takahiro Hirofuchi */ +#include <linux/kcov.h> #include <linux/kthread.h> #include <linux/slab.h> @@ -261,7 +262,9 @@ int vhci_rx_loop(void *data) if (usbip_event_happened(ud)) break; + kcov_remote_start_common(ud->kcov_handle); vhci_rx_pdu(ud); + kcov_remote_stop(); } return 0; diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index be37aec250c2..966f1f5cafb1 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -4,6 +4,7 @@ * Copyright (C) 2015-2016 Nobuo Iwata */ +#include <linux/kcov.h> #include <linux/kthread.h> #include <linux/file.h> #include <linux/net.h> @@ -378,11 +379,12 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n", devid, speed, usb_speed_string(speed)); - vdev->devid = devid; - vdev->speed = speed; - vdev->ud.sockfd = sockfd; - vdev->ud.tcp_socket = socket; - vdev->ud.status = VDEV_ST_NOTASSIGNED; + vdev->devid = devid; + vdev->speed = speed; + vdev->ud.sockfd = sockfd; + vdev->ud.tcp_socket = socket; + vdev->ud.status = VDEV_ST_NOTASSIGNED; + vdev->ud.kcov_handle = kcov_common_handle(); spin_unlock(&vdev->ud.lock); spin_unlock_irqrestore(&vhci->lock, flags);