Re: [PATCHv2 bpf-next] samples/bpf: add xdp program on egress for xdp_redirect_map

From: Jesper Dangaard Brouer <brouer@redhat.com>
To: Hangbin Liu <liuhangbin@gmail.com>
Cc: "Yonghong Song" <yhs@fb.com>,
	bpf@vger.kernel.org, netdev@vger.kernel.org,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Tariq Toukan" <tariqt@mellanox.com>,
	"Maciej Fijalkowski" <maciej.fijalkowski@intel.com>,
	brouer@redhat.com
Subject: Re: [PATCHv2 bpf-next] samples/bpf: add xdp program on egress for xdp_redirect_map
Date: Mon, 30 Nov 2020 10:32:08 +0100	[thread overview]
Message-ID: <20201130103208.6d5305e2@carbon> (raw)
In-Reply-To: <20201130075107.GB277949@localhost.localdomain>

On Mon, 30 Nov 2020 15:51:07 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:

> On Thu, Nov 26, 2020 at 10:31:56PM -0800, Yonghong Song wrote:
> > > index 35e16dee613e..8bdec0865e1d 100644
> > > --- a/samples/bpf/xdp_redirect_map_user.c
> > > +++ b/samples/bpf/xdp_redirect_map_user.c
> > > @@ -21,12 +21,13 @@
> > >   static int ifindex_in;
> > >   static int ifindex_out;
> > > -static bool ifindex_out_xdp_dummy_attached = true;
> > > +static bool ifindex_out_xdp_dummy_attached = false;
> > > +static bool xdp_prog_attached = false;  
> > 
> > Maybe xdp_devmap_prog_attached? Feel xdp_prog_attached
> > is too generic since actually it controls xdp_devmap program
> > attachment.  
> 
> Hi Yonghong,
> 
> Thanks for your comments. As Jesper replied, The 2nd xdp_prog on egress
> doesn't tell us if the redirect was successful. So the number is meaningless.

Well, I would not say the counter is meaningless.  It true that 2nd
devmap xdp_prog doesn't tell us if the redirect was successful, which
means that your description/(understanding) of the counter was wrong.

I still think it is relevant to have a counter for packets processed by
this 2nd xdp_prog, just to make is visually clear that the 2nd xdp-prog
attached (to devmap entry) is running.  The point is that QA is using
these programs.

The lack of good output from this specific sample have cause many
bugzilla cases for me.  BZ cases that requires going back and forth a
number of times, before figuring out how the prog was (mis)used.  This
is why other samples like xdp_rxq_info and xdp_redirect_cpu have such a
verbose output, which in-practice have helped many times on QA issues.

> I plan to write a example about vlan header modification based on egress
> index. I will post the patch later.

I did notice the internal thread you had with Toke.  I still think it
will be more simple to modify the Ethernet mac addresses.  Adding a
VLAN id tag is more work, and will confuse benchmarks.  You are
increasing the packet size, which means that you NIC need to spend
slightly more time sending this packet (3.2 nanosec at 10Gbit/s), which
could falsely be interpreted as cost of 2nd devmap XDP-program.

(Details: these 3.2 ns will not be visible for smaller packets, because
the minimum Ethernet frame size will hide this, but I've experience this
problem with larger frames on real switch hardware (Juniper), where
ingress didn't have VLAN-tag and egress we added VLAN-tag with XDP, and
then switch buffer slowly increased until overflow).

As Alexei already pointed out, you assignment is to modify the packet
in the 2nd devmap XDP-prog.  Why: because you need to realize that this
will break your approach to multicast in your previous patchset.
(Yes, the offlist patch I gave you, that move running 2nd devmap
XDP-prog to a later stage, solved this packet-modify issue).

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