Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Haiyang Zhang <haiyangz@microsoft.com>
To: Jakub Kicinski <jakub.kicinski@netronome.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 20:01:11 +0000
Message-ID: <DM6PR21MB133767862C27C874A882A4E1CA610@DM6PR21MB1337.namprd21.prod.outlook.com> (raw)
In-Reply-To: <20191029125308.78b52511@cakuba.hsd1.ca.comcast.net>



> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Tuesday, October 29, 2019 3:53 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
> 
> 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 :)
I will.

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

Sure I will document the recommended way of install xdp prog.

Thanks,
- Haiyang


  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
2019-10-29 20:01         ` Haiyang Zhang [this message]
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=DM6PR21MB133767862C27C874A882A4E1CA610@DM6PR21MB1337.namprd21.prod.outlook.com \
    --to=haiyangz@microsoft.com \
    --cc=davem@davemloft.net \
    --cc=jakub.kicinski@netronome.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