netdev - Re: [Intel-wired-lan] [PATCH v2 12/15] ice: Add stats and ethtool support

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:   Thu, 15 Mar 2018 21:41:53 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>,
        Jakub Kicinski <kubakici@...pl>,
        Netdev <netdev@...r.kernel.org>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [Intel-wired-lan] [PATCH v2 12/15] ice: Add stats and ethtool
 support

On Thu, 15 Mar 2018 17:50:10 -0700
Alexander Duyck <alexander.duyck@...il.com> wrote:

> On Thu, Mar 15, 2018 at 4:52 PM, Stephen Hemminger
> <stephen@...workplumber.org> wrote:
> > On Thu, 15 Mar 2018 16:47:59 -0700
> > Anirudh Venkataramanan <anirudh.venkataramanan@...el.com> wrote:
> >  
> >> +
> >> +static const struct ice_stats ice_gstrings_vsi_stats[] = {
> >> +     ICE_VSI_STAT("tx_unicast", eth_stats.tx_unicast),
> >> +     ICE_VSI_STAT("rx_unicast", eth_stats.rx_unicast),
> >> +     ICE_VSI_STAT("tx_multicast", eth_stats.tx_multicast),
> >> +     ICE_VSI_STAT("rx_multicast", eth_stats.rx_multicast),
> >> +     ICE_VSI_STAT("tx_broadcast", eth_stats.tx_broadcast),
> >> +     ICE_VSI_STAT("rx_broadcast", eth_stats.rx_broadcast),
> >> +     ICE_VSI_STAT("tx_bytes", eth_stats.tx_bytes),
> >> +     ICE_VSI_STAT("rx_bytes", eth_stats.rx_bytes),
> >> +     ICE_VSI_STAT("rx_discards", eth_stats.rx_discards),
> >> +     ICE_VSI_STAT("tx_errors", eth_stats.tx_errors),
> >> +     ICE_VSI_STAT("tx_linearize", tx_linearize),
> >> +     ICE_VSI_STAT("rx_unknown_protocol", eth_stats.rx_unknown_protocol),
> >> +     ICE_VSI_STAT("rx_alloc_fail", rx_buf_failed),
> >> +     ICE_VSI_STAT("rx_pg_alloc_fail", rx_page_failed),
> >> +};
> >> +  
> >
> > Ignoring feedback from maintainers is unlikely to help get your driver adopted.  
> 
> Your feedback wasn't ignored, the netdev stats are gone. I double
> checked and there was this in addition to the netdev stats before so I
> think the suggestion to remove the netdev stats was just taken
> literally.
> 
> The VSI is a slightly different entity from the netdev itself. A
> netdev can be backed by a VSI in the case of the PF, but the VSI can
> be used in other ways such as what we did in i40e where we were using
> it to spawn queue groups to work with mqprio as a filter target and in
> that case the queue groups wouldn't have a netdev directly associated
> with them so in that case it might make sense to leave these as
> separate stats.
> 
> - Alex

Sorry I was confused because eth_stats and thought is was a copy
of net_stats. This looks good.

After reading ice_stat_update40 I can see why you needed
to know starting point.

Since ice_fetch_u64_stats_per_ring is called only by ice_update_vsi_ring_stats,
and the update is only done by watchdog and when stats are fetched;
it doesn't look like you need to use the _irq variant of u64_stats_fetch_begin.
That would save having to disable local irq's while handling stats.


Acked-by: Stephen Hemminger <stephen@...workplumber.org>