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

[Branden Bonaby <brandonbonaby94@gmail.com>]

On Mon, Sep 02, 2019 at 06:31:04PM +0000, Michael Kelley wrote:
> From: Branden Bonaby <brandonbonaby94@gmail.com> Sent: Wednesday, August 28, 2019 9:24 PM
> > 
> > Introduce user specified latency in the packet reception path
> > By exposing the test parameters as part of the debugfs channel
> > attributes. We will control the testing state via these attributes.
> > 
> > Signed-off-by: Branden Bonaby <brandonbonaby94@gmail.com>
> > ---
> > diff --git a/Documentation/ABI/testing/debugfs-hyperv
> > b/Documentation/ABI/testing/debugfs-hyperv
> > new file mode 100644
> > index 000000000000..0903e6427a2d
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/debugfs-hyperv
> > @@ -0,0 +1,23 @@
> > +What:           /sys/kernel/debug/hyperv/<UUID>/fuzz_test_state
> > +Date:           August 2019
> > +KernelVersion:  5.3
> 
> Given the way the timing works for adding code to the Linux kernel,
> the earliest your new code will be included is 5.4.  The merge window
> for 5.4 will open in two weeks, so your code would need to be accepted
> by then to be included in 5.4.  Otherwise, it won't make it until the next
> merge window, which would be for 5.5.  I would suggest changing this
> (and the others below) to 5.4 for now, but you might have to change to
> 5.5 if additional revisions are needed.
> 

I see, I'll keep this in mind when submitting the further revisions
thanks

> > diff --git a/drivers/hv/Makefile b/drivers/hv/Makefile
> > index a1eec7177c2d..d754bd86ca47 100644
> > --- a/drivers/hv/Makefile
> > +++ b/drivers/hv/Makefile
> > @@ -2,6 +2,7 @@
> >  obj-$(CONFIG_HYPERV)		+= hv_vmbus.o
> >  obj-$(CONFIG_HYPERV_UTILS)	+= hv_utils.o
> >  obj-$(CONFIG_HYPERV_BALLOON)	+= hv_balloon.o
> > +obj-$(CONFIG_HYPERV_TESTING)	+= hv_debugfs.o
> 
> There's an additional complexity here that I failed to describe to
> you in my previous comments.  :-(    The above line makes the
> hv_debugfs code part of the main Linux OS image -- i.e., the
> vmlinuz file that gets installed into /boot, such that if hv_debugfs
> is built, it is always loaded into the memory of the running system.
> That's OK, but we should consider the alternative, which is to
> make the hv_debugfs code part of the hv_vmbus module that
> is specified a bit further down in the same Makefile.   A module
> is installed into /lib/modules/<kernel version>/kernel, and
> is only loaded into memory on the running system if something
> actually references code in the module.  This approach helps avoid
> loading code into memory that isn't actually used.
> 
> Whether code is built as a separate module or is just built into
> the main vmlinuz file is also controlled by the .config file setting.
> For example, CONFIG_HYPERV=m says to treat hv_vmbus as a
> separate module, while CONFIG_HYPERV=y says to build the
> hv_vmbus module directly into the vmlinuz file even though the
> Makefile constructs it as a module.
> 
> Making hv_debugfs part of the hv_vmbus module is generally better
> than just building it into the main vmlinuz because it's best to include
> only things that must always be present in the main vmlinuz file.
> hv_debugfs doesn't have any reason it needs to always be present.
> By comparison, hv_balloon is included in the main vmlinuz, which
> might be due to it dealing with memory mgmt and needing to be
> in the vmlinuz image, but I'm not sure.
> 
> You can make hv_debugfs part of the hv_vmbus module with this line
> placed after the line specifying hv_vmbus_y, instead of the line you
> currently have:
> 
> hv_vmbus-$(CONFIG_HYPERV_TESTING)       += hv_debugfs.o
> 

Ah, I see now. That makes sense I'll go ahead and make the adjustments

thanks

> > +
> > +static int hv_debugfs_delay_set(void *data, u64 val)
> > +{
> > +	int ret = 0;
> > +
> > +	if (val >= 1 && val <= 1000)
> > +		*(u32 *)data = val;
> > +	else if (val == 0)
> > +		*(u32 *)data = 0;
> 
> I think this "else if" clause can be folded into the first
> "if" statement by just checking "val >= 0".
> 

good call, I'll fold it into one statement 

> 
> > +/* Remove dentry associated with released hv device */
> > +void hv_debug_rm_dev_dir(struct hv_device *dev)
> > +{
> > +	if (!IS_ERR(hv_debug_root))
> > +		debugfs_remove_recursive(dev->debug_dir);
> > +}
> 
> This function is the first of five functions that are called by
> code outside of hv_debugfs.c.   You probably saw the separate
> email from the kbuild test robot showing a build error on each
> of these five functions.  Here's why.
> 
> When CONFIG_HYPERV=m is set, and hv_vmbus is built as a
> module, there are references to the five functions from the
> module to your hv_debugfs that is built as core code in
> vmlinuz.  By default, Linux does not allow such core code to
> be called by modules.   Core code must add a statement like:
> 
> EXPORT_SYMBOL_GPL(hv_debug_rm_dev_dir);
> 
> to allow the module to call it.   This gives the code writer
> a very minimal amount of scope control.  However, if you build 
> hv_debugfs as part of the module hv_vmbus, and the only
> references to the five functions are within the module hv_vmbus,
> then the EXPORT statements aren't needed because all
> references are internal to the hv_vmbus module.  But if
> you envision a function like hv_debug_delay_test() being
> called by other Hyper-V drivers that are outside the
> hv_vmbus module, then you will need the EXPORT statement
> at least for that function.
> 
> You probably have your .config file setup with
> CONFIG_HYPERV=y.  In that case, the hv_vmbus module is
> built-in to the kernel, so you didn't get the errors reported
> by the kbuild test robot.  When testing new code, it's a
> good practice to build with the HYPERV settings set to
> 'm' to make sure that any issues with module references
> get flushed out and fixed.
> 
> Michael

Oh I see, I'll test it out as a module so I can fix this. As for
exporting hv_debug_delay_test() It might come in handy later but
I think for now I should focus on just the ones in the hv_vmbus
module for now. 

thanks