Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Haiyang Zhang <haiyangz@microsoft.com>
Cc: "sashal@kernel.org" <sashal@kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	KY Srinivasan <kys@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"olaf@aepfle.de" <olaf@aepfle.de>, vkuznets <vkuznets@redhat.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
Date: Tue, 29 Oct 2019 12:53:08 -0700
Message-ID: <20191029125308.78b52511@cakuba.hsd1.ca.comcast.net> (raw)
In-Reply-To: <DM6PR21MB1337547067BE5E52DFE05E20CA610@DM6PR21MB1337.namprd21.prod.outlook.com>

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.

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-28 21:06 [PATCH net-next, 0/4] hv_netvsc: Add XDP support and some error handling fixes Haiyang Zhang
2019-10-28 21:07 ` [PATCH net-next, 1/4] hv_netvsc: Fix error handling in netvsc_set_features() Haiyang Zhang
2019-10-28 21:07 ` [PATCH net-next, 2/4] hv_netvsc: Fix error handling in netvsc_attach() Haiyang Zhang
2019-11-01 20:42   ` Markus Elfring
2019-11-04 15:08     ` Haiyang Zhang
2019-10-28 21:07 ` [PATCH net-next, 3/4] hv_netvsc: Add XDP support Haiyang Zhang
2019-10-28 21:33   ` Jakub Kicinski
2019-10-29 19:17     ` Haiyang Zhang
2019-10-29 19:53       ` Jakub Kicinski [this message]
2019-10-29 20:01         ` Haiyang Zhang
2019-10-29 21:59       ` Stephen Hemminger
2019-10-29 22:08         ` Haiyang Zhang
2019-10-28 21:07 ` [PATCH net-next, 4/4] hv_netvsc: Update document for " Haiyang Zhang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191029125308.78b52511@cakuba.hsd1.ca.comcast.net \
    --to=jakub.kicinski@netronome.com \
    --cc=davem@davemloft.net \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olaf@aepfle.de \
    --cc=sashal@kernel.org \
    --cc=sthemmin@microsoft.com \
    --cc=vkuznets@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git