RE: [PATCH v5 1/2] drivers: hv: vmbus: Introduce latency testing

[Michael Kelley]

From: Branden Bonaby <brandonbonaby94@xxxxxxxxx> 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