Re: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing
On Fri, Aug 23, 2019 at 04:44:09PM +, Michael Kelley wrote: > From: Branden Bonaby Sent: Thursday, August 22, > 2019 8:39 PM > > > > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c > > > > index 09829e15d4a0..c9c63a4033cd 100644 > > > > --- a/drivers/hv/connection.c > > > > +++ b/drivers/hv/connection.c > > > > @@ -357,6 +357,9 @@ void vmbus_on_event(unsigned long data) > > > > > > > > trace_vmbus_on_event(channel); > > > > > > > > +#ifdef CONFIG_HYPERV_TESTING > > > > + hv_debug_delay_test(channel, INTERRUPT_DELAY); > > > > +#endif /* CONFIG_HYPERV_TESTING */ > > > > > > You are following Vitaly's suggestion to use #ifdef's so no code is > > > generated when HYPERV_TESTING is not enabled. However, this > > > direct approach to using #ifdef's really clutters the code and makes > > > it harder to read and follow. The better approach is to use the > > > #ifdef in the include file where the functions are defined. If > > > HYPERV_TESTING is not enabled, provide a #else that defines > > > the function with an empty implementation for which the compiler > > > will generate no code. An as example, see the function definition > > > for hyperv_init() in arch/x86/include/asm/mshyperv.h. There are > > > several functions treated similarly in that include file. > > > > > > > I checked out the code in arch/x86/include/asm/mshyperv.h, after > > thinking about it, I'm wondering if it would be better just to have > > two files one called hv_debugfs.c and the other hyperv_debugfs.h. > > I could put the code definitions in hv_debugfs.c and at the top > > include the hyperv_debugfs.h file which would house the declarations > > of these functions under the ifdef. Then like you alluded too use > > an #else statement that would have the null implementations of the > > above functions. Then put an #include "hyperv_debugfs.h" in the > > hyperv_vmbus.h file. I figured instead of putting the code directly > > into the vmbus_drv.c file it might be best to put them in a seperate > > file like hv_debugfs.c. This way when we start adding more tests we > > don't bloat the vmbus_drv.c file unnecessarily. The hv_debugfs.c > > file would have the #ifdef CONFIG_HYPERV_TESTING at the top so if > > its not enabled those null implementations in "hyperv_debugfs.h" > > woud kick in anywhere that included the hyperv_vmbus.h file which > > is what we want. > > > > what do you think? > > > > I'll preface my comments by saying that how code gets structured > into files is always a bit of a judgment call. The goal is to group code > into sensible chunks to make it easy to understand and to make it > easy to modify and extend later. The latter is a prediction about the > future, which may or may not be accurate. For the former, what's > "easy to understand," is often in the eye of the beholder. So you may > get different opinions from different reviewers. > > That said, I like the idea of a separate hv_debugfs.c file to contain > the implementation of the various functions you have added to > provide the fuzzing capability. I'm less convinced about the value > of a separate hyperv_debugfs.h file. I think you have one big > #ifdef CONFIG_HYPERV_TESTING followed by the declarations of > the functions in hv_debugfs.c, followed by #else and null > implementations of those functions. This is 20 lines of code or so, > and could easily go in hyperv_vmbus.h. > > For the new hv_debugfs.c, you can avoid the need for > #ifdef CONFIG_HYPERV_TESTING by modifying the Makefile in > drivers/hv so that hv_debugfs.o is built only if CONFIG_HYPERV_TESTING > is defined. Look at the current Makefile to see how this is done > with CONFIG_HYPERV_UTILS and CONFIG_HYPERV_BALLOON. > > Michael > I see, that does make sense, I'll go ahead and add these changes. thanks branden bonaby
RE: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing
From: Branden Bonaby Sent: Thursday, August 22, 2019 8:39 PM > > > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c > > > index 09829e15d4a0..c9c63a4033cd 100644 > > > --- a/drivers/hv/connection.c > > > +++ b/drivers/hv/connection.c > > > @@ -357,6 +357,9 @@ void vmbus_on_event(unsigned long data) > > > > > > trace_vmbus_on_event(channel); > > > > > > +#ifdef CONFIG_HYPERV_TESTING > > > + hv_debug_delay_test(channel, INTERRUPT_DELAY); > > > +#endif /* CONFIG_HYPERV_TESTING */ > > > > You are following Vitaly's suggestion to use #ifdef's so no code is > > generated when HYPERV_TESTING is not enabled. However, this > > direct approach to using #ifdef's really clutters the code and makes > > it harder to read and follow. The better approach is to use the > > #ifdef in the include file where the functions are defined. If > > HYPERV_TESTING is not enabled, provide a #else that defines > > the function with an empty implementation for which the compiler > > will generate no code. An as example, see the function definition > > for hyperv_init() in arch/x86/include/asm/mshyperv.h. There are > > several functions treated similarly in that include file. > > > > I checked out the code in arch/x86/include/asm/mshyperv.h, after > thinking about it, I'm wondering if it would be better just to have > two files one called hv_debugfs.c and the other hyperv_debugfs.h. > I could put the code definitions in hv_debugfs.c and at the top > include the hyperv_debugfs.h file which would house the declarations > of these functions under the ifdef. Then like you alluded too use > an #else statement that would have the null implementations of the > above functions. Then put an #include "hyperv_debugfs.h" in the > hyperv_vmbus.h file. I figured instead of putting the code directly > into the vmbus_drv.c file it might be best to put them in a seperate > file like hv_debugfs.c. This way when we start adding more tests we > don't bloat the vmbus_drv.c file unnecessarily. The hv_debugfs.c > file would have the #ifdef CONFIG_HYPERV_TESTING at the top so if > its not enabled those null implementations in "hyperv_debugfs.h" > woud kick in anywhere that included the hyperv_vmbus.h file which > is what we want. > > what do you think? > I'll preface my comments by saying that how code gets structured into files is always a bit of a judgment call. The goal is to group code into sensible chunks to make it easy to understand and to make it easy to modify and extend later. The latter is a prediction about the future, which may or may not be accurate. For the former, what's "easy to understand," is often in the eye of the beholder. So you may get different opinions from different reviewers. That said, I like the idea of a separate hv_debugfs.c file to contain the implementation of the various functions you have added to provide the fuzzing capability. I'm less convinced about the value of a separate hyperv_debugfs.h file. I think you have one big #ifdef CONFIG_HYPERV_TESTING followed by the declarations of the functions in hv_debugfs.c, followed by #else and null implementations of those functions. This is 20 lines of code or so, and could easily go in hyperv_vmbus.h. For the new hv_debugfs.c, you can avoid the need for #ifdef CONFIG_HYPERV_TESTING by modifying the Makefile in drivers/hv so that hv_debugfs.o is built only if CONFIG_HYPERV_TESTING is defined. Look at the current Makefile to see how this is done with CONFIG_HYPERV_UTILS and CONFIG_HYPERV_BALLOON. Michael
Re: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing
> > endmenu
> > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> > index 09829e15d4a0..c9c63a4033cd 100644
> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -357,6 +357,9 @@ void vmbus_on_event(unsigned long data)
> >
> > trace_vmbus_on_event(channel);
> >
> > +#ifdef CONFIG_HYPERV_TESTING
> > + hv_debug_delay_test(channel, INTERRUPT_DELAY);
> > +#endif /* CONFIG_HYPERV_TESTING */
>
> You are following Vitaly's suggestion to use #ifdef's so no code is
> generated when HYPERV_TESTING is not enabled. However, this
> direct approach to using #ifdef's really clutters the code and makes
> it harder to read and follow. The better approach is to use the
> #ifdef in the include file where the functions are defined. If
> HYPERV_TESTING is not enabled, provide a #else that defines
> the function with an empty implementation for which the compiler
> will generate no code. An as example, see the function definition
> for hyperv_init() in arch/x86/include/asm/mshyperv.h. There are
> several functions treated similarly in that include file.
>
I checked out the code in arch/x86/include/asm/mshyperv.h, after
thinking about it, I'm wondering if it would be better just to have
two files one called hv_debugfs.c and the other hyperv_debugfs.h.
I could put the code definitions in hv_debugfs.c and at the top
include the hyperv_debugfs.h file which would house the declarations
of these functions under the ifdef. Then like you alluded too use
an #else statement that would have the null implementations of the
above functions. Then put an #include "hyperv_debugfs.h" in the
hyperv_vmbus.h file. I figured instead of putting the code directly
into the vmbus_drv.c file it might be best to put them in a seperate
file like hv_debugfs.c. This way when we start adding more tests we
don't bloat the vmbus_drv.c file unnecessarily. The hv_debugfs.c
file would have the #ifdef CONFIG_HYPERV_TESTING at the top so if
its not enabled those null implementations in "hyperv_debugfs.h"
woud kick in anywhere that included the hyperv_vmbus.h file which
is what we want.
what do you think?
>
> > do {
> > void (*callback_fn)(void *);
> >
> > diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> > index 362e70e9d145..edf14f596d8c 100644
> > --- a/drivers/hv/hyperv_vmbus.h
> > +++ b/drivers/hv/hyperv_vmbus.h
> > @@ -357,4 +357,24 @@ enum hvutil_device_state {
> > HVUTIL_DEVICE_DYING, /* driver unload is in progress */
> > };
> >
> > +#ifdef CONFIG_HYPERV_TESTING
> > +#include
> > +#include
> > +#include
>
> Generally #include files should go at the top of the file, even if they
> are only needed conditionally.
>
I see , will change
> > +#define TESTING "hyperv"
>
> I'm not seeing what this line is for, or how it is used.
I used it as the top level name for the dentry that
would appear in debugfs but now I realize its actually
not needed, so i'll remove this.
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -926,6 +926,21 @@ struct vmbus_channel {
> > * full outbound ring buffer.
> > */
> > u64 out_full_first;
> > +
> > +#ifdef CONFIG_HYPERV_TESTING
> > + /* enabling/disabling fuzz testing on the channel (default is false)*/
> > + bool fuzz_testing_state;
> > +
> > + /* Interrupt delay will delay the guest from emptying the ring buffer
> > +* for a specific amount of time. The delay is in microseconds and will
> > +* be between 1 to a maximum of 1000, its default is 0 (no delay).
> > +* The Message delay will delay guest reading on a per message basis
> > +* in microseconds between 1 to 1000 with the default being 0
> > +* (no delay).
> > +*/
> > + u32 fuzz_testing_interrupt_delay;
> > + u32 fuzz_testing_message_delay;
> > +#endif /* CONFIG_HYPERV_TESTING */
>
> For fields in a data structure like this, you don't have much choice
> but to put the #ifdef directly inline. However, for small fields like this
> and where the data structure isn't size sensitive, you could consider
> omitting the #ifdef and just always including the fields even when
> HYPERV_TESTING is not enabled. I don't have a strong preference
> either way.
>
I'll take the ifdefs out since the fields aren't too big
RE: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing
From: Branden Bonaby Sent: Monday, August 19, 2019
7:45 PM
>
> Introduce user specified latency in the packet reception path.
>
> Signed-off-by: Branden Bonaby
> ---
> Changes in v2:
> - Add #ifdef in Kconfig file so test code will not interfere
>with non-test code.
> - Move test code functions for delay to hyperv_vmbus header
>file.
> - Wrap test code under #ifdef statement.
>
> drivers/hv/Kconfig| 7 +++
> drivers/hv/connection.c | 3 +++
> drivers/hv/hyperv_vmbus.h | 20
> drivers/hv/ring_buffer.c | 7 +++
> include/linux/hyperv.h| 21 +
> 5 files changed, 58 insertions(+)
>
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 9a59957922d4..d97437ba0626 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -29,4 +29,11 @@ config HYPERV_BALLOON
> help
> Select this option to enable Hyper-V Balloon driver.
>
> +config HYPERV_TESTING
> +bool "Hyper-V testing"
> +default n
> +depends on HYPERV && DEBUG_FS
> +help
> + Select this option to enable Hyper-V vmbus testing.
> +
> endmenu
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 09829e15d4a0..c9c63a4033cd 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -357,6 +357,9 @@ void vmbus_on_event(unsigned long data)
>
> trace_vmbus_on_event(channel);
>
> +#ifdef CONFIG_HYPERV_TESTING
> + hv_debug_delay_test(channel, INTERRUPT_DELAY);
> +#endif /* CONFIG_HYPERV_TESTING */
You are following Vitaly's suggestion to use #ifdef's so no code is
generated when HYPERV_TESTING is not enabled. However, this
direct approach to using #ifdef's really clutters the code and makes
it harder to read and follow. The better approach is to use the
#ifdef in the include file where the functions are defined. If
HYPERV_TESTING is not enabled, provide a #else that defines
the function with an empty implementation for which the compiler
will generate no code. An as example, see the function definition
for hyperv_init() in arch/x86/include/asm/mshyperv.h. There are
several functions treated similarly in that include file.
> do {
> void (*callback_fn)(void *);
>
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 362e70e9d145..edf14f596d8c 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -357,4 +357,24 @@ enum hvutil_device_state {
> HVUTIL_DEVICE_DYING, /* driver unload is in progress */
> };
>
> +#ifdef CONFIG_HYPERV_TESTING
> +#include
> +#include
> +#include
Generally #include files should go at the top of the file, even if they
are only needed conditionally.
> +#define TESTING "hyperv"
I'm not seeing what this line is for, or how it is used.
> +
> +enum delay {
> + INTERRUPT_DELAY = 0,
> + MESSAGE_DELAY = 1,
> +};
> +
> +int hv_debug_delay_files(struct hv_device *dev, struct dentry *root);
> +int hv_debug_add_dev_dir(struct hv_device *dev);
> +void hv_debug_rm_dev_dir(struct hv_device *dev);
> +void hv_debug_rm_all_dir(void);
> +void hv_debug_set_dir_dentry(struct hv_device *dev, struct dentry *root);
> +void hv_debug_delay_test(struct vmbus_channel *channel, enum delay
> delay_type);
> +
This is where you could put a #else and the null implementation of
the above functions.
> +#endif /* CONFIG_HYPERV_TESTING */
> +
> #endif /* _HYPERV_VMBUS_H */
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 9a03b163cbbd..51adda23b398 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -396,6 +396,10 @@ struct vmpacket_descriptor *hv_pkt_iter_first(struct
> vmbus_channel *channel)
> struct hv_ring_buffer_info *rbi = &channel->inbound;
> struct vmpacket_descriptor *desc;
>
> +#ifdef CONFIG_HYPERV_TESTING
> + hv_debug_delay_test(channel, MESSAGE_DELAY);
> +#endif /* CONFIG_HYPERV_TESTING */
> +
> if (hv_pkt_iter_avail(rbi) < sizeof(struct vmpacket_descriptor))
> return NULL;
>
> @@ -421,6 +425,9 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
> u32 packetlen = desc->len8 << 3;
> u32 dsize = rbi->ring_datasize;
>
> +#ifdef CONFIG_HYPERV_TESTING
> + hv_debug_delay_test(channel, MESSAGE_DELAY);
> +#endif /* CONFIG_HYPERV_TESTING */
> /* bump offset to next potential packet */
> rbi->priv_read_index += packetlen + VMBUS_PKT_TRAILER;
> if (rbi->priv_read_index >= dsize)
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 6256cc34c4a6..6bf8ef5c780c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -926,6 +926,21 @@ struct vmbus_channel {
>* full outbound ring buffer.
>*/
> u64 out_full_first;
> +
> +#ifdef CONFIG_HYPERV_TESTING
> + /* enabling/disabling fuzz testing on the channel (default is false)*/
> + bool fuzz_testing_state;
> +
> + /* Interrupt delay will de
[PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing
Introduce user specified latency in the packet reception path.
Signed-off-by: Branden Bonaby
---
Changes in v2:
- Add #ifdef in Kconfig file so test code will not interfere
with non-test code.
- Move test code functions for delay to hyperv_vmbus header
file.
- Wrap test code under #ifdef statement.
drivers/hv/Kconfig| 7 +++
drivers/hv/connection.c | 3 +++
drivers/hv/hyperv_vmbus.h | 20
drivers/hv/ring_buffer.c | 7 +++
include/linux/hyperv.h| 21 +
5 files changed, 58 insertions(+)
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 9a59957922d4..d97437ba0626 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -29,4 +29,11 @@ config HYPERV_BALLOON
help
Select this option to enable Hyper-V Balloon driver.
+config HYPERV_TESTING
+bool "Hyper-V testing"
+default n
+depends on HYPERV && DEBUG_FS
+help
+ Select this option to enable Hyper-V vmbus testing.
+
endmenu
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 09829e15d4a0..c9c63a4033cd 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -357,6 +357,9 @@ void vmbus_on_event(unsigned long data)
trace_vmbus_on_event(channel);
+#ifdef CONFIG_HYPERV_TESTING
+ hv_debug_delay_test(channel, INTERRUPT_DELAY);
+#endif /* CONFIG_HYPERV_TESTING */
do {
void (*callback_fn)(void *);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 362e70e9d145..edf14f596d8c 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -357,4 +357,24 @@ enum hvutil_device_state {
HVUTIL_DEVICE_DYING, /* driver unload is in progress */
};
+#ifdef CONFIG_HYPERV_TESTING
+#include
+#include
+#include
+#define TESTING "hyperv"
+
+enum delay {
+ INTERRUPT_DELAY = 0,
+ MESSAGE_DELAY = 1,
+};
+
+int hv_debug_delay_files(struct hv_device *dev, struct dentry *root);
+int hv_debug_add_dev_dir(struct hv_device *dev);
+void hv_debug_rm_dev_dir(struct hv_device *dev);
+void hv_debug_rm_all_dir(void);
+void hv_debug_set_dir_dentry(struct hv_device *dev, struct dentry *root);
+void hv_debug_delay_test(struct vmbus_channel *channel, enum delay delay_type);
+
+#endif /* CONFIG_HYPERV_TESTING */
+
#endif /* _HYPERV_VMBUS_H */
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 9a03b163cbbd..51adda23b398 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -396,6 +396,10 @@ struct vmpacket_descriptor *hv_pkt_iter_first(struct
vmbus_channel *channel)
struct hv_ring_buffer_info *rbi = &channel->inbound;
struct vmpacket_descriptor *desc;
+#ifdef CONFIG_HYPERV_TESTING
+ hv_debug_delay_test(channel, MESSAGE_DELAY);
+#endif /* CONFIG_HYPERV_TESTING */
+
if (hv_pkt_iter_avail(rbi) < sizeof(struct vmpacket_descriptor))
return NULL;
@@ -421,6 +425,9 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
u32 packetlen = desc->len8 << 3;
u32 dsize = rbi->ring_datasize;
+#ifdef CONFIG_HYPERV_TESTING
+ hv_debug_delay_test(channel, MESSAGE_DELAY);
+#endif /* CONFIG_HYPERV_TESTING */
/* bump offset to next potential packet */
rbi->priv_read_index += packetlen + VMBUS_PKT_TRAILER;
if (rbi->priv_read_index >= dsize)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 6256cc34c4a6..6bf8ef5c780c 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -926,6 +926,21 @@ struct vmbus_channel {
* full outbound ring buffer.
*/
u64 out_full_first;
+
+#ifdef CONFIG_HYPERV_TESTING
+ /* enabling/disabling fuzz testing on the channel (default is false)*/
+ bool fuzz_testing_state;
+
+ /* Interrupt delay will delay the guest from emptying the ring buffer
+* for a specific amount of time. The delay is in microseconds and will
+* be between 1 to a maximum of 1000, its default is 0 (no delay).
+* The Message delay will delay guest reading on a per message basis
+* in microseconds between 1 to 1000 with the default being 0
+* (no delay).
+*/
+ u32 fuzz_testing_interrupt_delay;
+ u32 fuzz_testing_message_delay;
+#endif /* CONFIG_HYPERV_TESTING */
};
static inline bool is_hvsock_channel(const struct vmbus_channel *c)
@@ -1166,6 +1181,12 @@ struct hv_device {
struct vmbus_channel *channel;
struct kset *channels_kset;
+
+#ifdef CONFIG_HYPERV_TESTING
+ /* place holder to keep track of the dir for hv device in debugfs */
+ struct dentry *debug_dir;
+#endif /* CONFIG_HYPERV_TESTING */
+
};
--
2.17.1
