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

[Jakub Kicinski]

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.