Message-ID: <1530920986.7664.5.camel@intel.com>
Date:   Fri, 6 Jul 2018 23:49:55 +0000
From:   "Waskiewicz Jr, Peter" <peter.waskiewicz.jr@...el.com>
To:     "alexei.starovoitov@...il.com" <alexei.starovoitov@...il.com>
CC:     "Duyck, Alexander H" <alexander.h.duyck@...el.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "saeedm@...lanox.com" <saeedm@...lanox.com>,
        "brouer@...hat.com" <brouer@...hat.com>,
        "borkmann@...earbox.net" <borkmann@...earbox.net>,
        "tariqt@...lanox.com" <tariqt@...lanox.com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "saeedm@....mellanox.co.il" <saeedm@....mellanox.co.il>
Subject: Re: [RFC bpf-next 2/6] net: xdp: RX meta data infrastructure

On Fri, 2018-07-06 at 16:38 -0700, Alexei Starovoitov wrote:
> On Fri, Jul 06, 2018 at 08:44:24PM +0000, Waskiewicz Jr, Peter wrote:
> > On Fri, 2018-07-06 at 09:30 -0700, Alexei Starovoitov wrote:
> > > On Thu, Jul 05, 2018 at 10:18:23AM -0700, Jakub Kicinski wrote:
> > > > 
> > > > I'm also not 100% on board with the argument that "future" FW
> > > > can
> > > > reshuffle things whatever way it wants to.  Is the assumption
> > > > that
> > > > future ASICs/FW will be designed to always use the "blessed"
> > > > BTF
> > > > format?  Or will it be reconfigurable at runtime?
> > > 
> > > let's table configuration of metadata aside for a second.
> > 
> > I agree that this should/could be NIC-specific and shouldn't weigh
> > on
> > the metadata interface between the drivers and XDP layer.
> > 
> > > Describing metedata layout in BTF allows NICs to disclose
> > > everything
> > > NIC has to users in a standard and generic way.
> > > Whether firmware is reconfigurable on the fly or has to reflashed
> > > and hw powercycled to have new md layout (and corresponding BTF
> > > description)
> > > is a separate discussion.
> > > Saeed's proposal introduces the concept of 'offset' inside
> > > 'struct
> > > xdp_md_info'
> > > to reach 'hash' value in metadata.
> > > Essentially it's a run-time way to access 'hash' instead of
> > > build-
> > > time.
> > > So bpf program would need two loads to read csum or hash field
> > > instead of one.
> > > With BTF the layout of metadata is known to the program at build-
> > > time.
> > > 
> > > To reiterate the proposal:
> > > - driver+firmware keep layout of the metadata in BTF format
> > > (either
> > > in the driver
> > >   or driver can read it from firmware)
> > > - 'bpftool read-metadata-desc eth0 > md_desc.h' command will
> > > query
> > > the driver and
> > >   generate normal C header file based on BTF in the given NIC
> > > - user does #include "md_desc.h" and bpf program can access md-
> > > >csum
> > > or md->hash
> > >   with direct single load out of metadata area in front of the
> > > packet
> > 
> > This piece is where I'd like to discuss more.  When we discussed
> > this
> > in Seoul, the initial proposal was a static struct that we'd try to
> > hammer out a common layout between the interested parties.  That
> > obviously wasn't going to scale, and we wanted to pursue something
> > more
> > dynamic.  But I thought the goal was the XDP/eBPF program wouldn't
> > want
> > to care what the underlying device is, and could just ask for
> > metadata
> > that it's interested in.  With this approach, your eBPF program is
> > now
> > bound/tied to the NIC/driver, and if you switched to a differen
> > NIC/driver combo, then you'd have to rewrite part of your eBPF
> > program
> > to comprehend that.  I thought we were trying to avoid that.
> 
> It looks to me that NICs have a lot more differences instead of
> common pieces.
> If we focus the architecture on making common things as generic
> as possible we miss the bigger picture of covering distinct
> and unique features that hw provides.
> 
> In the last email when I said "would be good to standardize at least
> a few common fields"
> I meant that it make sense only at the later phases.
> I don't think we should put things into uapi upfront.
> Otherwise we will end up with likely useless fields.
> Like vlan field. Surely a lot of nics can store it into metadata, but
> why?
> bpf program can easily read it from the packet.
> What's the benefit of adding it to md?
> This metadata concept we're discussing in the context of program
> acceleration.
> If metadata doesn't give perf benefits it should not be done by
> firmware, it should not be supported by the driver and certainly
> should not be part of uapi.
> Hence I'd like to see pure BTF description without any
> common/standard
> fields across the drivers and figure out what (if anything) is useful
> to accelerate xdp programs (and af_xdp in the future).

I guess I didn't write my response very well before, apologies.  I
actually really like the BTF description and the dynamic-ness of it. 
It solves lots of problems and keeps things out of the UAPI.

What I was getting at was using the BTF description and dumping a
header file to be used in the program to be loaded.  If we have an
application developer include that header, it will be the Intel BTF
description, or Mellanox BTF description, etc.  I'm fine with that, but
it makes your program not as portable, since you'd need to get a
different header and recompile it if you change the NIC.  Unless I'm
missing something.

> 
> > Our proposed approach (still working on something ready to RFC) is
> > to
> > provide a method for the eBPF program to send a struct of requested
> > hints down to the driver on load.  If the driver can provide the
> > hints,
> > then that'd be how they'd be laid out in the metadata.  If it can't
> > provide them, we'd probably reject the program loading, or discuss
> > providing a software fallback (I know this is an area of
> > contention).
> 
> I don't think that approach can work.
> My .02 is the only useful acceleration feature out if intel nics is
> an ability to parse the packet and report it to the xdp program
> as a node id in the parse graph. As far as I know that is pretty
> unique
> to intel. Configuration of that is a different matter.
> Other things like hash are interesting, but we will quickly get
> into rss spec definition if we go standardization route now instead
> of later phases.
> Therefore I'd rather let firmware+driver define its own BTF
> description
> that says here is 'hash' of the packet hw can provide and it's fine
> that this hash may be over different tuples depending on the nic
> and even version of the firmware in that nic.
> We need to see performance numbers and benefits for real world
> xdp programs before drilling into specific semantics of the hash.

I agree that "useful" fields should be targeted.  I can see hash
getting more interesting though if one can specify what parts of the n-
tuples get included in the hash, or if we want hashes for inner packets
or outer headers if things are tunneled.  But that can be revisited
down the road.

I also agree that changing the configuration of the underlying parser
or pipeline to generate the metadata is a completely separate
discussion.

> 
> > I suppose we could get there with the rewriting mechanism described
> > below, but that'd be a tough sell to set a bit of ABI for metadata,
> > then change it to be potentially dynamic at runtime in the future.
> 
> Hmm. I don't see how future offset rewriting will break abi.
> It looks to me as natural extension that wouldn't break any apps
> that would be written for specific nic with given BTF description.
> With rewritting in place some progs would become portable. That's
> all.

What I meant here was if anything was put into the UAPI, then changing
to something more dynamic might make transitioning more challenging. 
And I can see now what the rewriting can do to help my previous
thoughts on the BTF description locking a program to a specific NIC. 
But I'd need to play with it to be convinced about the portability.

-PJ

Powered by blists - more mailing lists