| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM5PR21MB01375C24AE9ECD93DBE856C2D7AA0@DM5PR21MB0137.namprd21.prod.outlook.com>
Date: Wed, 21 Aug 2019 22:52:04 +0000
From: Michael Kelley <mikelley@...rosoft.com>
To: brandonbonaby94 <brandonbonaby94@...il.com>,
KY Srinivasan <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Stephen Hemminger <sthemmin@...rosoft.com>,
"sashal@...nel.org" <sashal@...nel.org>
CC: brandonbonaby94 <brandonbonaby94@...il.com>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing
From: Branden Bonaby <brandonbonaby94@...il.com> Sent: Monday, August 19, 2019 7:45 PM
>
> Introduce user specified latency in the packet reception path.
>
> Signed-off-by: Branden Bonaby <brandonbonaby94@...il.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
Powered by blists - more mailing lists