From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 20A2FC433DF for ; Thu, 20 Aug 2020 19:25:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BC5F3207BB for ; Thu, 20 Aug 2020 19:25:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597951520; bh=V1aWMg+xtiMStjO4PPnYgGhTjG+MOKqJVN3GZijnzxs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=yag4ZDHbescUPzEB0hx5lznd0eA5IOejYEop+ma0Zr9CeTTDEdIO+q+f+sDZBhlDy PyOCv2y2J/UQzv38cE+h6Fh1QwpRdgU5sM2oIFNPZ9S++fXkRZLQbkVzWm75W3TKpk p49PJeibbxTS1Rq8i+jm8NR01pmrrmGT0louweqM= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727870AbgHTTZS (ORCPT ); Thu, 20 Aug 2020 15:25:18 -0400 Received: from mail.kernel.org ([198.145.29.99]:34408 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726702AbgHTTZR (ORCPT ); Thu, 20 Aug 2020 15:25:17 -0400 Received: from kicinski-fedora-PC1C0HJN (unknown [163.114.132.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 0D4F5207BB; Thu, 20 Aug 2020 19:25:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597951516; bh=V1aWMg+xtiMStjO4PPnYgGhTjG+MOKqJVN3GZijnzxs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=WnYMnWeMLSNQ0QZqzlArCVIYFJGOxo2OL6bvSJY8jeyb3MVKAFLlGWBetHJip9sgi ilO8rk2vesf4geXh/UqCeJsOcNCaH00hXkWdQRSZY0Wl189XVbPeBDMdZ3Dwd7SLv+ o03C/D9Mj11g+wifaVMyHN7ukptYMruktcf4lgEY= Date: Thu, 20 Aug 2020 12:25:14 -0700 From: Jakub Kicinski To: David Thompson Cc: David Thompson , "netdev@vger.kernel.org" , "davem@davemloft.net" , Jiri Pirko , Asmaa Mnebhi Subject: Re: [PATCH net-next v2] Add Mellanox BlueField Gigabit Ethernet driver Message-ID: <20200820122514.5b552e42@kicinski-fedora-PC1C0HJN> In-Reply-To: References: <1596149638-23563-1-git-send-email-dthompson@mellanox.com> <20200730173059.7440e21c@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, 20 Aug 2020 18:51:39 +0000 David Thompson wrote: > > > + for (i = 0; i < priv->rx_q_entries; i++) { > > > + /* Allocate a receive buffer for this RX WQE. The DMA > > > + * form (dma_addr_t) of the receive buffer address is > > > + * stored in the RX WQE array (via 'rx_wqe_ptr') where > > > + * it is accessible by the GigE device. The VA form of > > > + * the receive buffer is stored in 'rx_buf[]' array in > > > + * the driver private storage for housekeeping. > > > + */ > > > + priv->rx_buf[i] = dma_alloc_coherent(priv->dev, > > > + > > MLXBF_GIGE_DEFAULT_BUF_SZ, > > > + &rx_buf_dma, > > > + GFP_KERNEL); > > > > Do the buffers have to be in coherent memory? That's kinda strange. > > > > Yes, the mlxbf_gige silicon block needs to be programmed with the > buffer's physical address so that the silicon logic can DMA incoming > packet data into the buffer. The kernel API "dma_alloc_coherent()" > meets the driver's requirements in that it returns a CPU-useable address > as well as a bus/physical address (used by silicon). It's highly unusual, all drivers I know use the streaming DMA interface. IDK what the performance implications for using coherent mappings on your platforms are, but I'd prefer if you took the more common approach. > > > +static void mlxbf_gige_get_ethtool_stats(struct net_device *netdev, > > > + struct ethtool_stats *estats, > > > + u64 *data) > > > +{ > > > + struct mlxbf_gige *priv = netdev_priv(netdev); > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > > Why do you take a lock around stats? > > I wrote the logic with a lock so that it implements an atomic "snapshot" > of the driver's statistics. This is useful since the standard TX/RX stats > are being incremented in packet completion logic triggered by the > NAPI framework. Do you see a disadvantage to using a lock here? The Linux APIs don't provide any "snapshot" guarantees, and you're stalling the datapath to read stats. > > > +static const struct net_device_ops mlxbf_gige_netdev_ops = { > > > + .ndo_open = mlxbf_gige_open, > > > + .ndo_stop = mlxbf_gige_stop, > > > + .ndo_start_xmit = mlxbf_gige_start_xmit, > > > + .ndo_set_mac_address = eth_mac_addr, > > > + .ndo_validate_addr = eth_validate_addr, > > > + .ndo_do_ioctl = mlxbf_gige_do_ioctl, > > > + .ndo_set_rx_mode = mlxbf_gige_set_rx_mode, > > > > You must report standard stats. > > Are you referring to the three possible methods that a driver > must use the implement support of standard stats reporting: > > From include/linux/netdevice.h --> > * void (*ndo_get_stats64)(struct net_device *dev, > * struct rtnl_link_stats64 *storage); > * struct net_device_stats* (*ndo_get_stats)(struct net_device *dev); > * Called when a user wants to get the network device usage > * statistics. Drivers must do one of the following: > * 1. Define @ndo_get_stats64 to fill in a zero-initialised > * rtnl_link_stats64 structure passed by the caller. > * 2. Define @ndo_get_stats to update a net_device_stats structure > * (which should normally be dev->stats) and return a pointer to > * it. The structure may be changed asynchronously only if each > * field is written atomically. > * 3. Update dev->stats asynchronously and atomically, and define > * neither operation. > > The mlxbf_gige driver has implemented #3 above, as there is logic > in the RX and TX completion handlers that increments RX/TX packet > and byte counts in the net_device->stats structure. Is that sufficient > for support of standard stats? You only update the basic stats. Please take a look at all members, some of the hw stats will probably fit there, and other HW errors need to be added to the cumulative rx/tx error stats.