netdev - RE: [PATCH V1 net-next 1/3] net: ena: ethtool: Add new device statistics

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] [day] [month] [year] [list]

Message-ID: <86e83120b1224a94a0ce2cecec7a441c@EX13D11EUB002.ant.amazon.com>
Date:   Wed, 19 Aug 2020 12:37:21 +0000
From:   "Jubran, Samih" <sameehj@...zon.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Machulsky, Zorik" <zorik@...zon.com>,
        "Matushevsky, Alexander" <matua@...zon.com>,
        "Bshara, Saeed" <saeedb@...zon.com>,
        "Wilson, Matt" <msw@...zon.com>,
        "Liguori, Anthony" <aliguori@...zon.com>,
        "Bshara, Nafea" <nafea@...zon.com>,
        "Tzalik, Guy" <gtzalik@...zon.com>,
        "Belgazal, Netanel" <netanel@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Herrenschmidt, Benjamin" <benh@...zon.com>,
        "Kiyanovski, Arthur" <akiyano@...zon.com>,
        "Dagan, Noam" <ndagan@...zon.com>
Subject: RE: [PATCH V1 net-next 1/3] net: ena: ethtool: Add new device statistics



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Tuesday, August 4, 2020 1:18 AM
> To: Jubran, Samih <sameehj@...zon.com>
> Cc: davem@...emloft.net; netdev@...r.kernel.org; Woodhouse, David
> <dwmw@...zon.co.uk>; Machulsky, Zorik <zorik@...zon.com>;
> Matushevsky, Alexander <matua@...zon.com>; Bshara, Saeed
> <saeedb@...zon.com>; Wilson, Matt <msw@...zon.com>; Liguori,
> Anthony <aliguori@...zon.com>; Bshara, Nafea <nafea@...zon.com>;
> Tzalik, Guy <gtzalik@...zon.com>; Belgazal, Netanel
> <netanel@...zon.com>; Saidi, Ali <alisaidi@...zon.com>; Herrenschmidt,
> Benjamin <benh@...zon.com>; Kiyanovski, Arthur
> <akiyano@...zon.com>; Dagan, Noam <ndagan@...zon.com>
> Subject: RE: [EXTERNAL] [PATCH V1 net-next 1/3] net: ena: ethtool: Add new
> device statistics
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> On Sat, 1 Aug 2020 14:21:28 +0000 sameehj@...zon.com wrote:
> > +     if (eni_stats_needed) {
> > +             ena_update_hw_stats(adapter);
> > +             for (i = 0; i < ENA_STATS_ARRAY_ENI(adapter); i++) {
> > +                     ena_stats = &ena_stats_eni_strings[i];
> > +
> > +                     ptr = (u64 *)((uintptr_t)&adapter->eni_stats +
> > +                             (uintptr_t)ena_stats->stat_offset);
> 
> In the kernel unsigned long is the type for doing maths on pointers.
Ack, will fix in V2.
> 
> > +                     ena_safe_update_stat(ptr, data++, &adapter->syncp);
> > +             }
> > +     }
> > +
> >       ena_queue_stats(adapter, &data);
> >       ena_dev_admin_queue_stats(adapter, &data);  }
> >
> > +static void ena_get_ethtool_stats(struct net_device *netdev,
> > +                               struct ethtool_stats *stats,
> > +                               u64 *data) {
> > +     struct ena_adapter *adapter = netdev_priv(netdev);
> > +
> > +     ena_get_stats(adapter, data, adapter->eni_stats_supported); }
> 
> Why the indirections? You always pass adapter->eni_stats_supported as a
> parameter, why not just use it directly?

please note that ena_dump_stats_ex(), ena_get_strings() and ena_get_stats()
are called with the parameter set to false. This is done to avoid sending an admin command, which sleeps, during the execution
of this function.
This function is called from the timer interrupt handler, which is an atomic context, and we want to avoid sleeping during
such context.
> 
> Other than the two nits, the set LGTM.