RE: [net-next 03/16] i40evf: Support RSS option in ethtool

-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Thursday, March 27, 2014 5:14 PM
To: Williams, Mitch A
Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com; Sullivan, Catherine
Subject: Re: [net-next 03/16] i40evf: Support RSS option in ethtool

On Wed, 2014-03-26 at 19:21 +0000, Williams, Mitch A wrote:

quoted

quoted

-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Monday, March 17, 2014 11:54 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Williams, Mitch A; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com; Sullivan, Catherine
Subject: Re: [net-next 03/16] i40evf: Support RSS option in ethtool

On Mon, 2014-03-17 at 05:45 -0700, Jeff Kirsher wrote:

quoted

From: Mitch Williams <redacted>

Add support for viewing and modifying RSS hash options and RSS hash
look-up table programming through ethtool. Because the lookup table is
so small for the VFs (only 16 registers), we don't bother to maintain

a

quoted

quoted

quoted

shadow table in memory, we just read and write the registers directly.

[...]

quoted

+static int i40evf_get_rss_hash_opts(struct i40evf_adapter *adapter,
+				    struct ethtool_rxnfc *cmd)
+{
+	cmd->data = 0;
+
+	/* Report default options for RSS on i40e */

But you're allowing them to be changed from the defaults, so this is
wrong.

Ben, I've got a question on this. Should the ETHTOOL_GRXFH option
report what the driver and device are capable of, or should it report
the current configuration?

It should report the current configuration.

quoted

If this option is supposed to report device capabilities, how can we
find out the current configuration? And if it's supposed to report the
current configuration, then how do we find out the device
capabilities?

You can't find out the device capabilities except by trial and error (or
driver-specific documentation).  Even if ETHTOOL_GRXFH did tell you
which fields *could* be included in the hash, the structure doesn't have
space to tell you which subsets are supported.

quoted

I've been wading through the kernel source, and I see both. So I'm
confused.

[...]

The first implementation was in niu, and in the absence of accompanying
documentation that is what I took to define the interface and what I
attempted to document in <linux/ethtool.h>.  If you think the current
kernel-doc comment on struct ethtool_rxnfc is not clear enough, please
do send a patch to improve the wording.

Thanks, Ben. I'll make sure that the i40evf driver does the right thing.

-Mitch

Ben.

--
Ben Hutchings
73.46% of all statistics are made up.