From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Toppins Subject: Re: [PATCH v1 net-next] tuntap: convert to 64-bit interface statistics Date: Thu, 19 Mar 2015 17:50:33 -0400 Message-ID: <550B44A9.5020402@cumulusnetworks.com> References: <1426794689-29943-1-git-send-email-jtoppins@cumulusnetworks.com> <1426801109.25985.8.camel@edumazet-glaptop2.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "David S. Miller" , netdev@vger.kernel.org, Curt Brune To: Eric Dumazet Return-path: Received: from mail-qg0-f44.google.com ([209.85.192.44]:33326 "EHLO mail-qg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750977AbbCSVuf (ORCPT ); Thu, 19 Mar 2015 17:50:35 -0400 Received: by qgfa8 with SMTP id a8so78055958qgf.0 for ; Thu, 19 Mar 2015 14:50:35 -0700 (PDT) In-Reply-To: <1426801109.25985.8.camel@edumazet-glaptop2.roam.corp.google.com> Sender: netdev-owner@vger.kernel.org List-ID: On 3/19/15 5:38 PM, Eric Dumazet wrote: > On Thu, 2015-03-19 at 12:51 -0700, Jonathan Toppins wrote: >> Signed-off-by: Curt Brune >> Signed-off-by: Jonathan Toppins >> --- >> drivers/net/tun.c | 54 ++++++++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 43 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index b96b94c..be8941a 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -65,6 +65,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -204,6 +205,9 @@ struct tun_struct { >> struct list_head disabled; >> void *security; >> u32 flow_count; >> + spinlock_t stat_lock; >> + struct u64_stats_sync stat_sync; >> + struct rtnl_link_stats64 stats64; >> }; >> >> static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val) >> @@ -751,6 +755,16 @@ static int tun_net_close(struct net_device *dev) >> return 0; >> } >> >> +static __always_inline void tun_stat64_inc(struct tun_struct *tun, u64 *stat, >> + size_t val) >> +{ >> + spin_lock_bh(&tun->stat_lock); >> + u64_stats_update_begin(&tun->stat_sync); >> + (*stat) += val; >> + u64_stats_update_end(&tun->stat_sync); >> + spin_unlock_bh(&tun->stat_lock); >> +} > > Ouch, one spin_lock_bh() ? Really ? > >> - tun->dev->stats.tx_packets++; >> - tun->dev->stats.tx_bytes += skb->len + vlan_hlen; >> + tun_stat64_inc(tun, &tun->stats64.tx_packets, 1); >> + tun_stat64_inc(tun, &tun->stats64.tx_bytes, skb->len + vlan_hlen); > > > So you take this spinlock twice ? > > Sorry, this is not good. > Hi Eric, thanks for the review. Would something like the following be preferable? spin_lock_bh(&tun->stat_lock); u64_stats_update_begin(&tun->stat_sync); &tun->stats64.tx_packets++; &tun->stats64.tx_bytes += skb->len + vlan_hlen; u64_stats_update_end(&tun->stat_sync); spin_unlock_bh(&tun->stat_lock); -Jon