netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ben Hutchings <bhutchings@solarflare.com>
To: Shreyas N Bhatewara <sbhatewara@vmware.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	pv-drivers@vmware.com
Subject: Re: [PATCH net-next 5/8] vmxnet3: Make ethtool handlers multiqueue aware
Date: Tue, 18 Jan 2011 16:05:14 +0000	[thread overview]
Message-ID: <1295366715.3537.9.camel@bwh-desktop> (raw)
In-Reply-To: <20110115005946.1064.97955.stgit@sbhatewara-dev1.eng.vmware.com>

On Fri, 2011-01-14 at 16:59 -0800, Shreyas N Bhatewara wrote:
> Show per-queue stats in ethtool -S output for vmxnet3 interface. Register dump
> of ethtool should dump registers for all tx and rx queues.
> 
> Signed-off-by: Shreyas N Bhatewara <sbhatewara@vmware.com>
> ---
>  drivers/net/vmxnet3/vmxnet3_ethtool.c |  259 ++++++++++++++++++---------------
>  1 files changed, 145 insertions(+), 114 deletions(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
> index 8e17fc8..d70cee1 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
> @@ -68,76 +68,78 @@ vmxnet3_set_rx_csum(struct net_device *netdev, u32 val)
>  static const struct vmxnet3_stat_desc
>  vmxnet3_tq_dev_stats[] = {
>  	/* description,         offset */
> -	{ "TSO pkts tx",        offsetof(struct UPT1_TxStats, TSOPktsTxOK) },
> -	{ "TSO bytes tx",       offsetof(struct UPT1_TxStats, TSOBytesTxOK) },
> -	{ "ucast pkts tx",      offsetof(struct UPT1_TxStats, ucastPktsTxOK) },
> -	{ "ucast bytes tx",     offsetof(struct UPT1_TxStats, ucastBytesTxOK) },
> -	{ "mcast pkts tx",      offsetof(struct UPT1_TxStats, mcastPktsTxOK) },
> -	{ "mcast bytes tx",     offsetof(struct UPT1_TxStats, mcastBytesTxOK) },
> -	{ "bcast pkts tx",      offsetof(struct UPT1_TxStats, bcastPktsTxOK) },
> -	{ "bcast bytes tx",     offsetof(struct UPT1_TxStats, bcastBytesTxOK) },
> -	{ "pkts tx err",        offsetof(struct UPT1_TxStats, pktsTxError) },
> -	{ "pkts tx discard",    offsetof(struct UPT1_TxStats, pktsTxDiscard) },
> +	{ "Tx Queue#",        0 },
> +	{ "  TSO pkts tx",	offsetof(struct UPT1_TxStats, TSOPktsTxOK) },
> +	{ "  TSO bytes tx",	offsetof(struct UPT1_TxStats, TSOBytesTxOK) },
[...]

I really don't like this.  You're making the assumption that these stats
will always be displayed as they are now by the ethtool command, but
that is not the only user of the ethtool API.

I expect that some people have scripts that involve reading ethtool
stats into a hash/dictionary.  (In fact, I wrote a diagnostic script for
Solarflare that does that.)  After this change to your driver, they
would get results from only one TX queue (with different names from
before).

So please:
- Don't use leading or trailing spaces in names
- Keep the global statistics, as most users will be more interested in
these
- If you think users actually want per-queue statistics, add them with
unique names (like bnx2x does)

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

  reply	other threads:[~2011-01-18 16:05 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-15  0:59 [PATCH net-next 0/8] vmxnet3 fixes and enhancements Shreyas N Bhatewara
2011-01-15  0:59 ` [PATCH net-next 1/8] vmxnet3: fix ring size update Shreyas N Bhatewara
2011-01-15  0:59 ` [PATCH net-next 2/8] vmxnet3: Preserve the MAC address configured by ifconfig Shreyas N Bhatewara
2011-01-15  0:59 ` [PATCH net-next 3/8] vmxnet3: Enable HW Rx VLAN stripping by default Shreyas N Bhatewara
2011-01-15  0:59 ` [PATCH net-next 4/8] vmxnet3: Provide required number of bytes in first SG buffer Shreyas N Bhatewara
2011-01-15  0:59 ` [PATCH net-next 5/8] vmxnet3: Make ethtool handlers multiqueue aware Shreyas N Bhatewara
2011-01-18 16:05   ` Ben Hutchings [this message]
2011-01-15  0:59 ` [PATCH net-next 6/8] vmxnet3: Disable napi in suspend, reenable in resume Shreyas N Bhatewara
2011-01-15  0:59 ` [PATCH net-next 7/8] vmxnet3: Add locking for access to command register Shreyas N Bhatewara
2011-01-15  1:00 ` [PATCH net-next 8/8] vmxnet3: Dont allocate extra MSI-x vectors Shreyas N Bhatewara
2011-01-16  5:21 ` [PATCH net-next 0/8] vmxnet3 fixes and enhancements David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1295366715.3537.9.camel@bwh-desktop \
    --to=bhutchings@solarflare.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pv-drivers@vmware.com \
    --cc=sbhatewara@vmware.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).