RE: [PATCH net-next, 3/4] hv_netvsc: Add XDP support

[Haiyang Zhang <haiyangz@microsoft.com>]

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