netdev - Re: [PATCHv2] net: usbnet: support 64bit stats in qmi

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]
Date:   Fri, 24 Mar 2017 15:42:38 +1000
From:   Greg Ungerer <gerg@...ux-m68k.org>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     bjorn@...k.no, oneukum@...e.com, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCHv2] net: usbnet: support 64bit stats in qmi_wwan driver

Hi Eric,

On 24/03/17 14:59, Eric Dumazet wrote:
> On Fri, 2017-03-24 at 11:27 +1000, Greg Ungerer wrote:
>> Add support for the net stats64 counters to the usbnet core and then to
>> the qmi_wwan driver.
>>
>> This is a strait forward addition of 64bit counters for RX and TX packets
>> and byte counts. It is done in the same style as for the other net drivers
>> that support stats64.
>>
>> The bulk of the change is to the usbnet core. Then it is trivial to use
>> that in the qmi_wwan.c driver. It would be very simple to extend this
>> support to other usbnet based drivers.
>>
>> The motivation to add this is that it is not particularly difficult to
>> get the RX and TX byte counts to wrap on 32bit platforms.
>>
>> Signed-off-by: Greg Ungerer <gerg@...ux-m68k.org>
>> ---
>>  drivers/net/usb/qmi_wwan.c |  1 +
>>  drivers/net/usb/usbnet.c   | 28 ++++++++++++++++++++++++++++
>>  include/linux/usb/usbnet.h | 12 ++++++++++++
>>  3 files changed, 41 insertions(+)
>>
>> v2: EXPORT symbol usbnet_get_stats64()
>>     rebase on top of net-next
>>
>> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
>> index 8056745..db12a66 100644
>> --- a/drivers/net/usb/qmi_wwan.c
>> +++ b/drivers/net/usb/qmi_wwan.c
>> @@ -251,6 +251,7 @@ static int qmi_wwan_mac_addr(struct net_device *dev, void *p)
>>  	.ndo_change_mtu		= usbnet_change_mtu,
>>  	.ndo_set_mac_address	= qmi_wwan_mac_addr,
>>  	.ndo_validate_addr	= eth_validate_addr,
>> +	.ndo_get_stats64	= usbnet_get_stats64,
>>  };
>>  
>>  /* using a counter to merge subdriver requests with our own into a
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 13d4ec5..4499d89 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -330,6 +330,11 @@ void usbnet_skb_return (struct usbnet *dev, struct sk_buff *skb)
>>  	dev->net->stats.rx_packets++;
>>  	dev->net->stats.rx_bytes += skb->len;
> 
> Why updating dev->net->stats.rx_{packets|bytes} and
> dev->stats.rx_{packets|bytes}  ?
> 
> Seems redundant.

The usbnet core is used by a number of drivers. This patch only
updates the qmi-wwan driver to use stats64. If you remove the
dev->stats.rx_* updates all those other driver users will have
no counts.


>> +	u64_stats_update_begin(&dev->stats.syncp);
>> +	dev->stats.rx_packets++;
>> +	dev->stats.rx_bytes += skb->len;
>> +	u64_stats_update_end(&dev->stats.syncp);
>> +
>>  	netif_dbg(dev, rx_status, dev->net, "< rx, len %zu, type 0x%x\n",
>>  		  skb->len + sizeof (struct ethhdr), skb->protocol);
>>  	memset (skb->cb, 0, sizeof (struct skb_data));
>> @@ -981,6 +986,23 @@ int usbnet_set_link_ksettings(struct net_device *net,
>>  }
>>  EXPORT_SYMBOL_GPL(usbnet_set_link_ksettings);
>>  
>> +void usbnet_get_stats64(struct net_device *net, struct rtnl_link_stats64 *stats)
>> +{
>> +	struct usbnet *dev = netdev_priv(net);
>> +	unsigned int start;
>> +
>> +	netdev_stats_to_stats64(stats, &net->stats);
>> +
>> +	do {
>> +		start = u64_stats_fetch_begin_irq(&dev->stats.syncp);
>> +		stats->rx_packets = dev->stats.rx_packets;
>> +		stats->rx_bytes = dev->stats.rx_bytes;
>> +		stats->tx_packets = dev->stats.tx_packets;
>> +		stats->tx_bytes = dev->stats.tx_bytes;
>> +	} while (u64_stats_fetch_retry_irq(&dev->stats.syncp, start));
>> +}
>> +EXPORT_SYMBOL_GPL(usbnet_get_stats64);
>> +
>>  u32 usbnet_get_link (struct net_device *net)
>>  {
>>  	struct usbnet *dev = netdev_priv(net);
>> @@ -1214,6 +1236,11 @@ static void tx_complete (struct urb *urb)
>>  	if (urb->status == 0) {
>>  		dev->net->stats.tx_packets += entry->packets;
>>  		dev->net->stats.tx_bytes += entry->length;
> 
> 
> Why updating dev->net->stats.tx_{packets|bytes} and
> dev->stats.tx_{packets|bytes}  ?
> 
>> +
>> +		u64_stats_update_begin(&dev->stats.syncp);
>> +		dev->stats.tx_packets += entry->packets;
>> +		dev->stats.tx_bytes += entry->length;
>> +		u64_stats_update_end(&dev->stats.syncp);
>>  	} else {
>>  		dev->net->stats.tx_errors++;
>>  
>> @@ -1658,6 +1685,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>>  	init_timer (&dev->delay);
>>  	mutex_init (&dev->phy_mutex);
>>  	mutex_init(&dev->interrupt_mutex);
>> +	u64_stats_init(&dev->stats.syncp);
>>  	dev->interrupt_count = 0;
>>  
>>  	dev->net = net;
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>> index e2b5691..d1501b8 100644
>> --- a/include/linux/usb/usbnet.h
>> +++ b/include/linux/usb/usbnet.h
>> @@ -22,6 +22,14 @@
>>  #ifndef	__LINUX_USB_USBNET_H
>>  #define	__LINUX_USB_USBNET_H
>>  
>> +struct usbnet_stats64 {
>> +	struct u64_stats_sync	syncp;
>> +	u64			rx_packets;
>> +	u64			rx_bytes;
>> +	u64			tx_packets;
>> +	u64			tx_bytes;
>> +};
>> +
> 
> You use the same syncp for TX and RX.
> 
> Do you have guarantee that TX and RX paths can not run concurrently (on
> different cpus) ?

Hmm, no, I don't there is any guarantee of that here.
I'll split rx and tx counters out with their own syncp.

Regards
Greg