Re: [PATCH RFC v4 bpf-next 09/11] tun: Support xdp in the Tx path for xdp_frames

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Mon, 2 Mar 2020 21:27:08 -0700
David Ahern <dahern@digitalocean.com> wrote:

> On 3/2/20 11:30 AM, Alexei Starovoitov wrote:
> > On Wed, Feb 26, 2020 at 08:20:11PM -0700, David Ahern wrote:  
> >> +
> >> +		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >> +		switch (act) {
> >> +		case XDP_TX:    /* for Tx path, XDP_TX == XDP_PASS */
> >> +			act = XDP_PASS;
> >> +			break;
> >> +		case XDP_PASS:
> >> +			break;
> >> +		case XDP_REDIRECT:
> >> +			/* fall through */
> >> +		default:
> >> +			bpf_warn_invalid_xdp_action(act);
> >> +			/* fall through */
> >> +		case XDP_ABORTED:
> >> +			trace_xdp_exception(tun->dev, xdp_prog, act);
> >> +			/* fall through */
> >> +		case XDP_DROP:
> >> +			break;
> >> +		}  
> > 
> > patch 8 has very similar switch. Can you share the code?  
> 
> Most likely; I'll take a look.
> 
> > 
> > I'm worried that XDP_TX is a silent alias to XDP_PASS.
> > What were the reasons to go with this approach?  
> 
> As I stated in the cover letter:
> 
> "XDP_TX on Rx means send the packet out the device it arrived
> on; given that, XDP_Tx for the Tx path is treated as equivalent to
> XDP_PASS - ie., continue on the Tx path."

I'm not really buying this.  IHMO XDP_PASS should mean continue on the
Tx path, as this is a tx/egress XDP hook.  I don't see a reason to
basically "remove" the XDP_TX return code at this point.

> > imo it's less error prone and extensible to warn on XDP_TX.
> > Which will mean that both XDP_TX and XDP_REDICT are not supported
> > for egress atm.  

I agree.

I more see "XDP_TX" as a mirror facility... maybe there is a use-case
for bouncing packets back in the TX/Egress hook? That is future work,
but not reason disable the option now.


> I personally don't care either way; I was going with the simplest
> concept from a user perspective.
> 
> > 
> > Patches 8 and 9 cover tun only. I'd like to see egress hook to be
> > implemented in at least one physical NIC. Pick any hw. Something
> > that handles real frames. Adding this hook to virtual NIC is easy,
> > but it doesn't demonstrate design trade-offs one would need to
> > think through by adding egress hook to physical nic. That's why I
> > think it's mandatory to have it as part of the patch set.
> > 
> > Patch 11 exposes egress to samples/bpf. It's nice, but without
> > selftests it's no go. All new features must be exercised as part of
> > selftests/bpf.  
> 
> Patches that exercise the rtnetlink uapi are fairly easy to do on
> single node; anything traffic related requires multiple nodes or
> namespace level capabilities.  Unless I am missing something that is
> why all current XDP tests ride on top of veth; veth changes are not
> part of this set.
> 
> So to be clear you are saying that all new XDP features require
> patches to a h/w nic, veth and whatever the author really cares about
> before new features like this go in?

I would say yes. XDP is founded for physical HW NICs, and we need to
show it makes sense for physical HW NICs.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer