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

[Branden Bonaby <brandonbonaby94@gmail.com>]

On Thu, Sep 19, 2019 at 10:52:41PM +0000, Michael Kelley wrote:
> From: Branden Bonaby <brandonbonaby94@gmail.com> 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