RE: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing

[Michael Kelley <mikelley@microsoft.com>]

From: Branden Bonaby <brandonbonaby94@gmail.com> Sent: Monday, August 19, 2019 7:45 PM
> 
> Introduce user specified latency in the packet reception path.
> 
> Signed-off-by: Branden Bonaby <brandonbonaby94@gmail.com>
> ---
> 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 <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>

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 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.

>  };
> 
>  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 */

Same here.

Michael

> +
>  };
> 
> 
> --
> 2.17.1