From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id AA03440875; Tue, 6 Aug 2024 20:37:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722976642; cv=none; b=e1UywqPh5Z+jweJs3fYBe26pJ/IfwukLn/m9ckGASEOM3LOI2fFlfHMY5HZ1YszePqpsS7xp5hjwfGYoA9OQAaz8a6/QUi97zpYr0e8cG3uL/922elDOQkdXYw4wGxe3QKRiB9DqYgn/oWmcpxFxOclUHHDa4mUGIPrMYg7bW7k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722976642; c=relaxed/simple; bh=g7QmdLe9IFWfz7C77+mSFv4ErgmVpHHeVChQmimHYG8=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=vEkR2Sih9hN6YqOQJbx3rNxW2RwuyeGL+6UAKwT7b9PkKvDpFV8FtRNM2N+B9FdB/S7+8WORFnPHGop5IOtyMZ7ukzXFfEbjrSL/rYMElPBMOam3iJvmVON2y9AbaXSV6AdxiNwTjboaPQY4+/dciggFe0imMTNhZHLGLbZsr4k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=X8YxGWgP; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="X8YxGWgP" Received: by smtp.kernel.org (Postfix) with ESMTPSA id F2EDAC32786; Tue, 6 Aug 2024 20:37:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1722976642; bh=g7QmdLe9IFWfz7C77+mSFv4ErgmVpHHeVChQmimHYG8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=X8YxGWgP2UMFZzmnwxFTZacJCBIWbq/z7d+2P/PY9Q5HIpRaRo5UUK629pTV5bU+K L3ODKR2S5C8Cly3RBZpnOtbw5A5RD74DNc4Aj4LRaY63F5SOWYn6WNAABfQVNaoSB4 FnE3lOClziNlgKZEPEYmJBdLpHfq+tGNeTW7non+xpfZ8H3Lr3TTodHiZ+fYNm7oFa r005iJn+WbIbLd0BIb+hBF88OkmE45EdIBQjJcPcfRT2D2o5UiOYQ/EiQLGXjGO1tH AfKdXhSddmFMc8kSMXFn4WyrWcVQtGQCNAtdpP2AnK1qNaK4TzeqntlXV0d0ds42aS r3Mb4dSi5D2ZA== Date: Tue, 6 Aug 2024 13:37:18 -0700 From: Jakub Kicinski To: Mina Almasry Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-alpha@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, sparclinux@vger.kernel.org, linux-trace-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org, bpf@vger.kernel.org, linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, Donald Hunter , "David S. Miller" , Eric Dumazet , Paolo Abeni , Jonathan Corbet , Richard Henderson , Ivan Kokshaysky , Matt Turner , Thomas Bogendoerfer , "James E.J. Bottomley" , Helge Deller , Andreas Larsson , Jesper Dangaard Brouer , Ilias Apalodimas , Steven Rostedt , Masami Hiramatsu , Mathieu Desnoyers , Arnd Bergmann , Steffen Klassert , Herbert Xu , David Ahern , Willem de Bruijn , Shuah Khan , Sumit Semwal , "Christian =?UTF-8?B?S8O2bmln?=" , Bagas Sanjaya , Christoph Hellwig , Nikolay Aleksandrov , Taehee Yoo , Pavel Begunkov , David Wei , Jason Gunthorpe , Yunsheng Lin , Shailend Chand , Harshitha Ramamurthy , Shakeel Butt , Jeroen de Borst , Praveen Kaligineedi , Willem de Bruijn , Kaiyuan Zhang , Daniel Vetter Subject: Re: [PATCH net-next v18 03/14] netdev: support binding dma-buf to netdevice Message-ID: <20240806133718.71a53a4f@kernel.org> In-Reply-To: <20240805212536.2172174-4-almasrymina@google.com> References: <20240805212536.2172174-1-almasrymina@google.com> <20240805212536.2172174-4-almasrymina@google.com> Precedence: bulk X-Mailing-List: linux-doc@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 5 Aug 2024 21:25:16 +0000 Mina Almasry wrote: > +/* Protected by rtnl_lock() */ > +static DEFINE_XARRAY_FLAGS(net_devmem_dmabuf_bindings, XA_FLAGS_ALLOC1); nit: global variable declarations before any code > +void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding) > +{ > + struct netdev_rx_queue *rxq; > + unsigned long xa_idx; > + unsigned int rxq_idx; > + > + if (binding->list.next) > + list_del(&binding->list); > + > + xa_for_each(&binding->bound_rxqs, xa_idx, rxq) { > + if (rxq->mp_params.mp_priv == binding) { > + rxq->mp_params.mp_priv = NULL; > + > + rxq_idx = get_netdev_rx_queue_index(rxq); > + > + netdev_rx_queue_restart(binding->dev, rxq_idx); Throw in a WARN_ON() around this, hopefully we'll get to addressing it later.. > + } > + } > + > + xa_erase(&net_devmem_dmabuf_bindings, binding->id); > + > + net_devmem_dmabuf_binding_put(binding); > +} > + > +int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > + struct net_devmem_dmabuf_binding *binding) > +{ > + struct netdev_rx_queue *rxq; > + u32 xa_idx; > + int err; > + > + if (rxq_idx >= dev->real_num_rx_queues) > + return -ERANGE; If we prevent binding to an inactive queue we should also prevent deactivation. Please take a look at the (two?) callers of ethtool_get_max_rxnfc_channel() and ethtool_get_max_rxfh_channel(). Wrap those into a new function for reading max active channel, and take mp binds into account as well (send the refactor separately from the series to avoid making it longer). > + rxq = __netif_get_rx_queue(dev, rxq_idx); > + if (rxq->mp_params.mp_priv) > + return -EEXIST; > + > + err = xa_alloc(&binding->bound_rxqs, &xa_idx, rxq, xa_limit_32b, > + GFP_KERNEL); > + if (err) > + return err; > + > + rxq->mp_params.mp_priv = binding; > + > + err = netdev_rx_queue_restart(dev, rxq_idx); > + if (err) > + goto err_xa_erase; > + > + return 0; > + > +err_xa_erase: > + rxq->mp_params.mp_priv = NULL; > + xa_erase(&binding->bound_rxqs, xa_idx); > + > + return err; > +} > +void dev_dmabuf_uninstall(struct net_device *dev) > +{ > + unsigned int i, count = dev->num_rx_queues; nit: why stash the value of num_rx_queues ? > + struct net_devmem_dmabuf_binding *binding; > + struct netdev_rx_queue *rxq; > + unsigned long xa_idx; > + > + for (i = 0; i < count; i++) { > + binding = dev->_rx[i].mp_params.mp_priv; > + if (binding) > + xa_for_each(&binding->bound_rxqs, xa_idx, rxq) > + if (rxq == &dev->_rx[i]) > + xa_erase(&binding->bound_rxqs, xa_idx); nit: Please use "continue", this is too deeply indented > + nla_for_each_attr_type(attr, NETDEV_A_DMABUF_QUEUES, > + genlmsg_data(info->genlhdr), > + genlmsg_len(info->genlhdr), rem) { > + err = nla_parse_nested( > + tb, ARRAY_SIZE(netdev_queue_id_nl_policy) - 1, attr, > + netdev_queue_id_nl_policy, info->extack); > + if (err < 0) > + goto err_unbind; > + > + rxq_idx = nla_get_u32(tb[NETDEV_A_QUEUE_ID]); How do we know this attribute is present? NL_REQ_ATTR_CHECK() > + err = net_devmem_bind_dmabuf_to_queue(netdev, rxq_idx, binding); > + if (err) > + goto err_unbind; > + } > + > + list_add(&binding->list, sock_binding_list); > + > + nla_put_u32(rsp, NETDEV_A_DMABUF_ID, binding->id); > + genlmsg_end(rsp, hdr); > + > + rtnl_unlock(); nit: for symmetry you should also unlock after list_add(), netlink msg alloc and prep are before rtnl_lock() > + return genlmsg_reply(rsp, info); > + > +err_unbind: > + net_devmem_unbind_dmabuf(binding); > +err_unlock: > + rtnl_unlock(); > +err_genlmsg_free: > + nlmsg_free(rsp); > + return err; > } > +void netdev_nl_sock_priv_init(struct list_head *priv) > +{ > + INIT_LIST_HEAD(priv); > +} > + > +void netdev_nl_sock_priv_destroy(struct list_head *priv) > +{ > + struct net_devmem_dmabuf_binding *binding; > + struct net_devmem_dmabuf_binding *temp; > + > + list_for_each_entry_safe(binding, temp, priv, list) { > + rtnl_lock(); > + net_devmem_unbind_dmabuf(binding); > + rtnl_unlock(); > + } > +} nit: move these before the subsys_initcall.. and what it calls