netdev - Re: [net-next v2] net: ifb: support ethtools stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211126134254.45bd82c2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 26 Nov 2021 13:42:54 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     xiangxia.m.yue@...il.com
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [net-next v2] net: ifb: support ethtools stats

On Fri, 26 Nov 2021 11:23:05 +0800 xiangxia.m.yue@...il.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@...il.com>
> 
> With this feature, we can use the ethtools to get tx/rx
> queues stats. This patch, introduce the ifb_update_q_stats
> helper to update the queues stats, and ifb_q_stats to simplify
> the codes. In future, we can add more metrics in ifb_q_stats.
> 
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: "David S. Miller" <davem@...emloft.net>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>

>  static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev);
>  static int ifb_open(struct net_device *dev);
>  static int ifb_close(struct net_device *dev);
>  
> +static inline void ifb_update_q_stats(struct ifb_q_stats *stats, int len)

Please remove the "inline" keywords, we prefer to leave the choice to
the compiler (plus it hides "unused function" warnings if the caller 
is ever removed).

> +static void ifb_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> +{
> +	u8 *p = buf;
> +	int i, j;
> +
> +	switch(stringset) {
> +	case ETH_SS_STATS:
> +		for (i = 0; i < dev->real_num_rx_queues; i++)
> +			for (j = 0; j < IFB_Q_STATS_LEN; j++)
> +				ethtool_sprintf(&p, "rx_queue_%u_%.18s",
> +						i, ifb_q_stats_desc[j].desc);
> +
> +		for (i = 0; i < dev->real_num_tx_queues; i++)
> +			for (j = 0; j < IFB_Q_STATS_LEN; j++)
> +				ethtool_sprintf(&p, "tx_queue_%u_%.18s",
> +						i, ifb_q_stats_desc[j].desc);
> +
> +		break;
> +	}
> +}
> +
> +static int ifb_get_sset_count(struct net_device *dev, int sset)
> +{
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		return IFB_Q_STATS_LEN * (dev->real_num_rx_queues +
> +		       dev->real_num_tx_queues);

Needs to align under opening bracket, try checkpatch --strict.

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static inline void ifb_fill_stats_data(u64 **data,
> +				       struct ifb_q_stats *q_stats)

another inline

The logic itself LGTM.