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  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]
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.

Powered by blists - more mailing lists