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=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,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 69D34CA9EAE for ; Tue, 29 Oct 2019 19:53:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 35EA02067D for ; Tue, 29 Oct 2019 19:53:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=netronome-com.20150623.gappssmtp.com header.i=@netronome-com.20150623.gappssmtp.com header.b="CL6xSLkC" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726535AbfJ2TxU (ORCPT ); Tue, 29 Oct 2019 15:53:20 -0400 Received: from mail-lf1-f68.google.com ([209.85.167.68]:39574 "EHLO mail-lf1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725870AbfJ2TxT (ORCPT ); Tue, 29 Oct 2019 15:53:19 -0400 Received: by mail-lf1-f68.google.com with SMTP id 195so11476631lfj.6 for ; Tue, 29 Oct 2019 12:53:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :organization:mime-version:content-transfer-encoding; bh=ELIuHAsLUdOptuJRjC0EGY1vsp+EwGqf0TMIjtj9lzo=; b=CL6xSLkCn8z+SPxn0Ox2Q6QupnXa+71PuXMVumYce/AdZ5SIw+naoKj16l8UR2SCHV 7i1haBetmU0frrYstQP06LJgQMQMft5ACDcvfsVzEKD5SAltBC2dnqXYH+wzgKuK6LO5 m9++MWlozUYf3gNf93unMZDWiaov7NrL4GYQ7kXkCp+vf6lNQ5H2MRE13ertgZP0+lCX UETuw+o2EjBHSJFVt+NjApYTc259y7RV4Hw/Uam5kGh18cz+Jf8dEASSJTMq5zlq3B2m uTAYA8f3f5+uKdzxBSaxaqgOGGjH6pJVUZnXIxnB2n7hw+9oVGzcp2DxG9WKNeHJ99XR 7CQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:organization:mime-version:content-transfer-encoding; bh=ELIuHAsLUdOptuJRjC0EGY1vsp+EwGqf0TMIjtj9lzo=; b=bbDe8JftWrbPPCnEgYbMGHLqjgIGwgisg92ESV2bMw1i6Q/3wQTkE+fad+Vd/3esZa eygyPWW+EO8fO8hH5VhEv0N8q4Hec15GDPXbUL1AihnojCL17uxYKeKFWeB3Ex/0npZm Lq3qzcuZYGVNMpw5KNgXe82W9CrwyAzY5oX0Rrfxz4DUapvFcUK9sVkkcw5IzDdchzJ/ bKHmtYaaNHsqgnvyiiYdGfE+GjcnZkxrOO+e6fCl+dwr8s28ruzSSh9wGbwfqndft5Ao WaQdcIoAX6YUQLdOp19X/SHlfQ1eMWy408gKk40q6FYB4tENuCmzmQH6kqWoIba4yIId ROFA== X-Gm-Message-State: APjAAAUctnFll3EBdVCYZBKGKvfWrv8LgVILwuRwSKTHJwFjJHNTmiPQ ywYzJePNfAnXImKr6E9y0972Qg== X-Google-Smtp-Source: APXvYqxoXomwuRDO/YmNQAylZagn+LTxZqxb/kzuIdcD9fkqeiGTej2ZyTO75cc3M2ivp48a1iK2vw== X-Received: by 2002:a05:6512:1de:: with SMTP id f30mr3570832lfp.176.1572378797083; Tue, 29 Oct 2019 12:53:17 -0700 (PDT) Received: from cakuba.hsd1.ca.comcast.net ([66.60.152.14]) by smtp.gmail.com with ESMTPSA id c5sm6465233ljd.57.2019.10.29.12.53.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Oct 2019 12:53:16 -0700 (PDT) Date: Tue, 29 Oct 2019 12:53:08 -0700 From: Jakub Kicinski To: Haiyang Zhang Cc: "sashal@kernel.org" , "linux-hyperv@vger.kernel.org" , "netdev@vger.kernel.org" , KY Srinivasan , Stephen Hemminger , "olaf@aepfle.de" , vkuznets , "davem@davemloft.net" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support Message-ID: <20191029125308.78b52511@cakuba.hsd1.ca.comcast.net> In-Reply-To: References: <1572296801-4789-1-git-send-email-haiyangz@microsoft.com> <1572296801-4789-4-git-send-email-haiyangz@microsoft.com> <20191028143322.45d81da4@cakuba.hsd1.ca.comcast.net> Organization: Netronome Systems, Ltd. MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-hyperv-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-hyperv@vger.kernel.org On Tue, 29 Oct 2019 19:17:25 +0000, Haiyang Zhang wrote: > > > +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog, > > > + struct netvsc_device *nvdev) > > > +{ > > > + struct bpf_prog *old_prog; > > > + int frag_max, i; > > > + > > > + old_prog = netvsc_xdp_get(nvdev); > > > + > > > + if (!old_prog && !prog) > > > + return 0; > > > > I think this case is now handled by the core. > Thanks for the reminder. I saw the code in dev_change_xdp_fd(), so the upper layer > doesn't call XDP_SETUP_PROG with old/new prog both NULL. > But this function is also called by other functions in our driver, like netvsc_detach(), > netvsc_remove(), etc. Instead of checking for NULL in each place, I still keep the check inside > netvsc_xdp_set(). I see. Makes sense on a closer look. BTW would you do me a favour and reformat this line: static struct netvsc_device_info *netvsc_devinfo_get (struct netvsc_device *nvdev) to look like this: static struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev) or static struct netvsc_device_info * netvsc_devinfo_get(struct netvsc_device *nvdev) Otherwise git diff gets confused about which function given chunk belongs to. (Incorrectly thinking your patch is touching netvsc_get_channels()). I spent few minutes trying to figure out what's going on there :) > > > > > + return -EOPNOTSUPP; > > > + } > > > + > > > + if (prog) { > > > + prog = bpf_prog_add(prog, nvdev->num_chn); > > > + if (IS_ERR(prog)) > > > + return PTR_ERR(prog); > > > + } > > > + > > > + for (i = 0; i < nvdev->num_chn; i++) > > > + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog); > > > + > > > + if (old_prog) > > > + for (i = 0; i < nvdev->num_chn; i++) > > > + bpf_prog_put(old_prog); > > > + > > > + return 0; > > > +} > > > + > > > +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog) > > > +{ > > > + struct netdev_bpf xdp; > > > + bpf_op_t ndo_bpf; > > > + > > > + ASSERT_RTNL(); > > > + > > > + if (!vf_netdev) > > > + return 0; > > > + > > > + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf; > > > + if (!ndo_bpf) > > > + return 0; > > > + > > > + memset(&xdp, 0, sizeof(xdp)); > > > + > > > + xdp.command = XDP_SETUP_PROG; > > > + xdp.prog = prog; > > > + > > > + return ndo_bpf(vf_netdev, &xdp); > > > > IMHO the automatic propagation is not a good idea. Especially if the > > propagation doesn't make the entire installation fail if VF doesn't > > have ndo_bpf. > > On Hyperv and Azure hosts, VF is always acting as a slave below netvsc. > And they are both active -- most data packets go to VF, but broadcast, > multicast, and TCP SYN packets go to netvsc synthetic data path. The synthetic > NIC (netvsc) is also a failover NIC when VF is not available. > We ask customers to only use the synthetic NIC directly. So propagation > of XDP setting to VF NIC is desired. > But, I will change the return code to error, so the entire installation fails if VF is > present but unable to set XDP prog. Okay, if I read the rest of the code correctly you also fail attach if xdp propagation failed? If that's the case and we return an error here on missing NDO, then the propagation could be okay. So the semantics are these: (a) install on virt - potentially overwrites the existing VF prog; (b) install on VF is not noticed by virt; (c) uninstall on virt - clears both virt and VF, regardless what program was installed on virt; (d) uninstall on VF does not propagate; Since you're adding documentation it would perhaps be worth stating there that touching the program on the VF is not supported/may lead to breakage, and users should only touch/configure the program on the virt.