Re: [PATCH v5 1/2] drivers: hv: vmbus: Introduce latency testing
On Thu, Sep 19, 2019 at 10:52:41PM +, Michael Kelley wrote:
> From: Branden Bonaby Sent: Thursday, September
> 12, 2019 7:32 PM
> >
> > +
> > +static int hv_debugfs_delay_set(void *data, u64 val)
> > +{
> > + int ret = 0;
> > +
> > + if (val >= 0 && val <= 1000)
> > + *(u32 *)data = val;
> > + else
> > + ret = -EINVAL;
> > +
> > + return ret;
> > +}
>
> I should probably quit picking at your code, but I'm going to
> do it one more time. :-)
>
> The above test for val >=0 is redundant as 'val' is declared
> as 'u64'. As an unsigned value, it will always be >= 0. More
> broadly, the above function could be written as follows
> with no loss of clarity. This accomplishes the same thing in
> only 4 lines of code instead of 6, and the main execution path
> is in the sequential execution flow, not in an 'if' statement.
>
> {
> if (val > 1000)
> return -EINVAL;
> *(u32 *)data = val;
> return 0;
> }
>
> Your code is correct as written, so this is arguably more a
> matter of style, but Linux generally likes to do things clearly
> and compactly with no extra motion.
>
Yea the less than 0 comparison isnt needed, so I'll update that
> +/* Delay buffer/message reads on a vmbus channel */
> > +void hv_debug_delay_test(struct vmbus_channel *channel, enum delay
> > delay_type)
> > +{
> > + struct vmbus_channel *test_channel =channel->primary_channel ?
> > + channel->primary_channel :
> > + channel;
> > + bool state = test_channel->fuzz_testing_state;
> > +
> > + if (state) {
> > + if (delay_type == 0)
> > + udelay(test_channel->fuzz_testing_interrupt_delay);
> > + else
> > + udelay(test_channel->fuzz_testing_message_delay);
>
> This 'if/else' statement got me thinking. You have an enum declared below
> that lists the two options -- INTERRUPT_DELAY or MESSAGE_DELAY. The
> implication is that we might add more options in the future. But the
> above 'if/else' statement isn't really set up to easily add more options, and
> the individual fields for fuzz_testing_interrupt_delay and
> fuzz_testing_message_delay mean adding more branches to the 'if/else'
> statement whenever a new DELAY type is added to the enum. And the
> same is true when adding the entries into debugfs. A more general
> solution might use arrays and loops, and treat the enum value as an
> index into an array of delay values. Extending to add another delay type
> could be as easy as adding another entry to the enum declaration.
>
> The current code is for the case where n=2 (i.e., two different delay
> types), and as such probably doesn't warrant the full index/looping
> treatment. But in the future, if we add additional delay types, we'll
> probably revise the code to do the index/looping approach.
>
> So to be clear, at this point I'm not asking you to change the existing
> code. My comments are more of an observation and something to
> think about in the future.
>
I do see your point, thanks for the input. I think since its just two
it might be better to leave it but it definitely makes sense.
> >
> > +enum delay {
> > + INTERRUPT_DELAY = 0,
> > + MESSAGE_DELAY = 1,
> > +};
> > +
>
> Michael
RE: [PATCH v5 1/2] drivers: hv: vmbus: Introduce latency testing
From: Branden Bonaby Sent: Thursday, September 12,
2019 7:32 PM
>
> +
> +static int hv_debugfs_delay_set(void *data, u64 val)
> +{
> + int ret = 0;
> +
> + if (val >= 0 && val <= 1000)
> + *(u32 *)data = val;
> + else
> + ret = -EINVAL;
> +
> + return ret;
> +}
I should probably quit picking at your code, but I'm going to
do it one more time. :-)
The above test for val >=0 is redundant as 'val' is declared
as 'u64'. As an unsigned value, it will always be >= 0. More
broadly, the above function could be written as follows
with no loss of clarity. This accomplishes the same thing in
only 4 lines of code instead of 6, and the main execution path
is in the sequential execution flow, not in an 'if' statement.
{
if (val > 1000)
return -EINVAL;
*(u32 *)data = val;
return 0;
}
Your code is correct as written, so this is arguably more a
matter of style, but Linux generally likes to do things clearly
and compactly with no extra motion.
> +/* Delay buffer/message reads on a vmbus channel */
> +void hv_debug_delay_test(struct vmbus_channel *channel, enum delay
> delay_type)
> +{
> + struct vmbus_channel *test_channel =channel->primary_channel ?
> + channel->primary_channel :
> + channel;
> + bool state = test_channel->fuzz_testing_state;
> +
> + if (state) {
> + if (delay_type == 0)
> + udelay(test_channel->fuzz_testing_interrupt_delay);
> + else
> + udelay(test_channel->fuzz_testing_message_delay);
This 'if/else' statement got me thinking. You have an enum declared below
that lists the two options -- INTERRUPT_DELAY or MESSAGE_DELAY. The
implication is that we might add more options in the future. But the
above 'if/else' statement isn't really set up to easily add more options, and
the individual fields for fuzz_testing_interrupt_delay and
fuzz_testing_message_delay mean adding more branches to the 'if/else'
statement whenever a new DELAY type is added to the enum. And the
same is true when adding the entries into debugfs. A more general
solution might use arrays and loops, and treat the enum value as an
index into an array of delay values. Extending to add another delay type
could be as easy as adding another entry to the enum declaration.
The current code is for the case where n=2 (i.e., two different delay
types), and as such probably doesn't warrant the full index/looping
treatment. But in the future, if we add additional delay types, we'll
probably revise the code to do the index/looping approach.
So to be clear, at this point I'm not asking you to change the existing
code. My comments are more of an observation and something to
think about in the future.
>
> +enum delay {
> + INTERRUPT_DELAY = 0,
> + MESSAGE_DELAY = 1,
> +};
> +
Michael
[PATCH v5 1/2] drivers: hv: vmbus: Introduce latency testing
Introduce user specified latency in the packet reception path
By exposing the test parameters as part of the debugfs channel
attributes. We will control the testing state via these attributes.
Signed-off-by: Branden Bonaby
---
changes in v5:
- As per Stephen's suggestion, Moved CONFIG_HYPERV_TESTING
to lib/Kconfig.debug.
- Fixed build issue reported by Kbuild, with Michael's
suggestion to make hv_debugfs part of the hv_vmbus
module.
- updated debugfs-hyperv to show kernel version 5.4
changes in v4:
- Combined v3 patch 2 into this patch, and changed the
commit description to reflect this.
- Moved debugfs code from "vmbus_drv.c" that was in
previous v3 patch 2, into a new file "debugfs.c" in
drivers/hv.
- Updated the Makefile to compile "debugfs.c" if
CONFIG_HYPERV_TESTING is enabled
- As per Michael's comments, added empty implementations
of the new functions, so the compiler will not generate
code when CONFIG_HYPERV_TESTING is not enabled.
- Added microseconds into description for files in
Documentation/ABI/testing/debugfs-hyperv.
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.
Documentation/ABI/testing/debugfs-hyperv | 23 +++
MAINTAINERS | 1 +
drivers/hv/Makefile | 1 +
drivers/hv/connection.c | 1 +
drivers/hv/hv_debugfs.c | 185 +++
drivers/hv/hyperv_vmbus.h| 31
drivers/hv/ring_buffer.c | 2 +
drivers/hv/vmbus_drv.c | 6 +
include/linux/hyperv.h | 19 +++
lib/Kconfig.debug| 7 +
10 files changed, 276 insertions(+)
create mode 100644 Documentation/ABI/testing/debugfs-hyperv
create mode 100644 drivers/hv/hv_debugfs.c
diff --git a/Documentation/ABI/testing/debugfs-hyperv
b/Documentation/ABI/testing/debugfs-hyperv
new file mode 100644
index ..4427503ec762
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-hyperv
@@ -0,0 +1,23 @@
+What: /sys/kernel/debug/hyperv//fuzz_test_state
+Date: August 2019
+KernelVersion: 5.4
+Contact:Branden Bonaby
+Description:Fuzz testing status of a vmbus device, whether its in an ON
+state or a OFF state
+Users: Debugging tools
+
+What:
/sys/kernel/debug/hyperv//delay/fuzz_test_buffer_interrupt_delay
+Date: August 2019
+KernelVersion: 5.4
+Contact:Branden Bonaby
+Description:Fuzz testing buffer interrupt delay value between 0 - 1000
+microseconds (inclusive).
+Users: Debugging tools
+
+What: /sys/kernel/debug/hyperv//delay/fuzz_test_message_delay
+Date: August 2019
+KernelVersion: 5.4
+Contact:Branden Bonaby
+Description:Fuzz testing message delay value between 0 - 1000 microseconds
+(inclusive).
+Users: Debugging tools
diff --git a/MAINTAINERS b/MAINTAINERS
index e7a47b5210fd..00831931eb22 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7468,6 +7468,7 @@ F:include/uapi/linux/hyperv.h
F: include/asm-generic/mshyperv.h
F: tools/hv/
F: Documentation/ABI/stable/sysfs-bus-vmbus
+F: Documentation/ABI/testing/debugfs-hyperv
HYPERBUS SUPPORT
M: Vignesh Raghavendra
diff --git a/drivers/hv/Makefile b/drivers/hv/Makefile
index a1eec7177c2d..94daf8240c95 100644
--- a/drivers/hv/Makefile
+++ b/drivers/hv/Makefile
@@ -9,4 +9,5 @@ CFLAGS_hv_balloon.o = -I$(src)
hv_vmbus-y := vmbus_drv.o \
hv.o connection.o channel.o \
channel_mgmt.o ring_buffer.o hv_trace.o
+hv_vmbus-$(CONFIG_HYPERV_TESTING) += hv_debugfs.o
hv_utils-y := hv_util.o hv_kvp.o hv_snapshot.o hv_fcopy.o hv_utils_transport.o
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 09829e15d4a0..4d4d40832846 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -357,6 +357,7 @@ void vmbus_on_event(unsigned long data)
trace_vmbus_on_event(channel);
+ hv_debug_delay_test(channel, INTERRUPT_DELAY);
do {
void (*callback_fn)(void *);
diff --git a/drivers/hv/hv_debugfs.c b/drivers/hv/hv_debugfs.c
new file mode 100644
index ..933080b51410
--- /dev/null
+++ b/drivers/hv/hv_debugfs.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Authors:
+ * Branden Bonaby
+ */
+
+#include
+#include
+#include
+#include
+
+#include "hyperv_vmbus.h"
+
+struct dentry *hv_debug_root;
+
+static int hv_debugfs_delay_get(void *data, u64 *val)
+{
+ *val = *(u32 *)data;
+ return 0;
+}
+
+static int hv_debugfs_delay_set(void *data, u64 val)
+{
+ int ret = 0;
+
+ if (val >= 0 && val <= 1000)
+ *(u32 *)data = val;
+ else
+
