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

[Michael Kelley <mikelley@microsoft.com>]

From: Branden Bonaby <brandonbonaby94@gmail.com> Sent: Thursday, August 22, 2019 8:39 PM
> > > 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.
> >
> 
> I checked out the code in arch/x86/include/asm/mshyperv.h, after
> thinking about it, I'm wondering if it would be better just to have
> two files one called hv_debugfs.c and the other hyperv_debugfs.h.
> I could put the code definitions in hv_debugfs.c and at the top
> include the hyperv_debugfs.h file which would house the declarations
> of these functions under the ifdef. Then like you alluded too use
> an #else statement that would have the null implementations of the
> above functions. Then put an #include "hyperv_debugfs.h" in the
> hyperv_vmbus.h file. I figured instead of putting the code directly
> into the vmbus_drv.c file it might be best to put them in a seperate
> file like hv_debugfs.c. This way when we start adding more tests we
> don't bloat the vmbus_drv.c file unnecessarily. The hv_debugfs.c
> file would have the #ifdef CONFIG_HYPERV_TESTING at the top so if
> its not enabled  those null implementations in "hyperv_debugfs.h"
> woud kick in anywhere that included the hyperv_vmbus.h file which
> is what we want.
> 
> what do you think?
> 

I'll preface my comments by saying that how code gets structured
into files is always a bit of a judgment call.  The goal is to group code
into sensible chunks to make it easy to understand and to make it
easy to modify and extend later.  The latter is a prediction about the
future, which may or may not be accurate.   For the former, what's
"easy to understand," is often in the eye of the beholder.  So you may
get different opinions from different reviewers.

That said, I like the idea of a separate hv_debugfs.c file to contain
the implementation of the various functions you have added to
provide the fuzzing capability.   I'm less convinced about the value
of a separate hyperv_debugfs.h file.   I think you have one big
#ifdef CONFIG_HYPERV_TESTING followed by the declarations of
the functions in hv_debugfs.c, followed by #else and null
implementations of those functions.  This is 20 lines of code or so,
and could easily go in hyperv_vmbus.h.

For the new hv_debugfs.c, you can avoid the need for
#ifdef CONFIG_HYPERV_TESTING by modifying the Makefile in
drivers/hv so that hv_debugfs.o is built only if CONFIG_HYPERV_TESTING
is defined.  Look at the current Makefile to see how this is done
with CONFIG_HYPERV_UTILS and CONFIG_HYPERV_BALLOON.

Michael