Re: [RFC bpf-next v3 6/8] flow_dissector: handle no-skb use case

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Stanislav Fomichev <sdf@fomichev.me>,
	Stanislav Fomichev <sdf@google.com>,
	Network Development <netdev@vger.kernel.org>,
	bpf@vger.kernel.org, David Miller <davem@davemloft.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	simon.horman@netronome.com, Willem de Bruijn <willemb@google.com>,
	Petar Penkov <peterpenkov96@gmail.com>
Subject: Re: [RFC bpf-next v3 6/8] flow_dissector: handle no-skb use case
Date: Tue, 26 Mar 2019 13:51:27 -0400	[thread overview]
Message-ID: <CAF=yD-KzG1E0kL3s4TfYX+T8Bc3VeZw9siKK8iDHAN1uyYDyLA@mail.gmail.com> (raw)
In-Reply-To: <20190326174802.jnhvtdephykfj6ku@ast-mbp>

On Tue, Mar 26, 2019 at 1:48 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Mar 26, 2019 at 09:45:29AM -0700, Stanislav Fomichev wrote:
> > On 03/25, Alexei Starovoitov wrote:
> > > On Sat, Mar 23, 2019 at 09:05:31AM -0700, Stanislav Fomichev wrote:
> > > > On 03/22, Alexei Starovoitov wrote:
> > > > > On Fri, Mar 22, 2019 at 06:19:57PM -0700, Stanislav Fomichev wrote:
> > > > > > Are we ok with breaking api in this case? I'm all in on removing this
> > > > > > extra information. We can always put it back if somebody complains (and
> > > > > > manually parse in eth_get_headlen case).
> > > > >
> > > > > Fine. That seems to be the only way forward to clean it all up.
> > > > > Could you submit patch 1 to bpf tree disallowing vlan fields?
> > > > > Patch 3 looks like candidate as well?
> > > > SGTM, will do. Let me also spend some time and do a simple test for
> > > > the vlan case, to make sure I didn't miss something important.
> > > > One question here though: would I need to wait for bpf and bpf-next
> > > > to re-merge to continues the series? Or we can cherry-pick those
> > > > patches to bpf-next as well (and git will work it out during the
> > > > merge)?
> > > >
> > > > > > We can still have protocol, because in both skb/skb-less cases we have
> > > > > > it.
> > > > >
> > > > > proto can work in both cases, but is it needed ? Does program benefit from it?
> > > > > The kernel side burns extra bytes by copying it and extra branches to handle it.
> > > > > May be drop it as well?
> > > > I feel like the program benefits from it, there is no need to go back and
> > > > re-parse that (and in the skb case, this data is already pulled). I was
> > > > also thinking about re-purposing flow_keys->n_proto for that (instead
> > > > of skb->protocol), so it functions as input and output, maybe that's a
> > > > more clear way to do it.
> > >
> > > Are you saying that skb-less and skb flow dissector progs are looking
> > > at different positions into the packet ?
> > No, sorry for confusion, they are both called to parse (optional) L2-vlan
> > and L3+ headers. However, with-skb case can be called with l2-vlan
> > parsed (post RFS) or with l2-vlan unparsed (RFS). The vlan is pulled in
> > __netif_receive_skb_core, but we can still invoke flow dissector prior to
> > that when doing RFS (get_rps_cpu).
> >
> > That's why have this 'if skb->vlan_present' check in the bpf_flow.c program
> > (and then also manually test for ETH_P_8021Q/ETH_P_8021AD).
> >
> > Let me try to post the patches to bpf tree somewhere this week, we
> > can discuss the API changes there.
>
> let's figure out what we disable for bpf/stable first.
> Sound like skb->protocol isn't helping.
> prog has to read it from eth header anyway. then let's drop it from ctx?

The C flow dissector does not parse link layer headers It starts with
proto either given as argument or derived from skb->protocol (or
skb->vlan_proto).

The BPF flow dissector should work the same. It is fine to pass the
data including ethernet header, but parsing can start at nhoff with
proto explicitly passed.

We should not assume Ethernet link layer.