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@xxxxxxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
