bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jamal Hadi Salim <jhs@mojatatu.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: netdev@vger.kernel.org, deb.chatterjee@intel.com,
	anjali.singhai@intel.com,  namrata.limaye@intel.com,
	tom@sipanda.io, mleitner@redhat.com,  Mahesh.Shirshyad@amd.com,
	Vipin.Jain@amd.com, tomasz.osinski@intel.com,  jiri@resnulli.us,
	xiyou.wangcong@gmail.com, davem@davemloft.net,
	 edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	vladbu@nvidia.com,  horms@kernel.org, khalidm@nvidia.com,
	toke@redhat.com, daniel@iogearbox.net,  victor@mojatatu.com,
	pctammela@mojatatu.com, dan.daly@intel.com,
	 andy.fingerhut@gmail.com, chris.sommers@keysight.com,
	mattyk@nvidia.com,  bpf@vger.kernel.org
Subject: Re: [PATCH net-next v12 00/15] Introducing P4TC (series 1)
Date: Wed, 28 Feb 2024 13:23:00 -0500	[thread overview]
Message-ID: <CAM0EoMnMrOAZ1iGocDDhVmoeY33fxZjiUEQc4yp0KJj8nASrAA@mail.gmail.com> (raw)
In-Reply-To: <65df6935db67e_2a12e2083b@john.notmuch>

On Wed, Feb 28, 2024 at 12:11 PM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Jamal Hadi Salim wrote:
> > This is the first patchset of two. In this patch we are submitting 15 which
> > cover the minimal viable P4 PNA architecture.
> >
> > __Description of these Patches__
> >
> > Patch #1 adds infrastructure for per-netns P4 actions that can be created on
> > as need basis for the P4 program requirement. This patch makes a small incision
> > into act_api. Patches 2-4 are minimalist enablers for P4TC and have no
> > effect the classical tc action (example patch#2 just increases the size of the
> > action names from 16->64B).
> > Patch 5 adds infrastructure support for preallocation of dynamic actions.
> >
> > The core P4TC code implements several P4 objects.
> > 1) Patch #6 introduces P4 data types which are consumed by the rest of the code
> > 2) Patch #7 introduces the templating API. i.e. CRUD commands for templates
> > 3) Patch #8 introduces the concept of templating Pipelines. i.e CRUD commands
> >    for P4 pipelines.
> > 4) Patch #9 introduces the action templates and associated CRUD commands.
> > 5) Patch #10 introduce the action runtime infrastructure.
> > 6) Patch #11 introduces the concept of P4 table templates and associated
> >    CRUD commands for tables.
> > 7) Patch #12 introduces runtime table entry infra and associated CU commands.
> > 8) Patch #13 introduces runtime table entry infra and associated RD commands.
> > 9) Patch #14 introduces interaction of eBPF to P4TC tables via kfunc.
> > 10) Patch #15 introduces the TC classifier P4 used at runtime.
> >
> > Daniel, please look again at patch #15.
> >
> > There are a few more patches (5) not in this patchset that deal with test
> > cases, etc.
> >
> > What is P4?
> > -----------
> >
> > The Programming Protocol-independent Packet Processors (P4) is an open source,
> > domain-specific programming language for specifying data plane behavior.
> >
> > The current P4 landscape includes an extensive range of deployments, products,
> > projects and services, etc[9][12]. Two major NIC vendors, Intel[10] and AMD[11]
> > currently offer P4-native NICs. P4 is currently curated by the Linux
> > Foundation[9].
> >
> > On why P4 - see small treatise here:[4].
> >
> > What is P4TC?
> > -------------
> >
> > P4TC is a net-namespace aware P4 implementation over TC; meaning, a P4 program
> > and its associated objects and state are attachend to a kernel _netns_ structure.
> > IOW, if we had two programs across netns' or within a netns they have no
> > visibility to each others objects (unlike for example TC actions whose kinds are
> > "global" in nature or eBPF maps visavis bpftool).
>
> [...]
>
> Although I appreciate a good amount of work went into building above I'll
> add my concerns here so they are not lost. These are architecture concerns
> not this line of code needs some tweak.
>
>  - It encodes a DSL into the kernel. Its unclear how we pick which DSL gets
>    pushed into the kernel and which do not. Do we take any DSL folks can code
>    up?
>    I would prefer a lower level  intermediate langauge. My view is this is
>    a lesson we should have learned from OVS. OVS had wider adoption and
>    still struggled in some ways my belief is this is very similar to OVS.
>    (Also OVS was novel/great at a lot of things fwiw.)
>
>  - We have a general purpose language in BPF that can implement the P4 DSL
>    already. I don't see any need for another set of code when the end goal
>    is running P4 in Linux network stack is doable. Typically we reject
>    duplicate things when they don't have concrete benefits.
>
>  - P4 as a DSL is not optimized for general purpose CPUs, but
>    rather hardware pipelines. Although it can be optimized for CPUs its
>    a harder problem. A review of some of the VPP/DPDK work here is useful.
>
>  - P4 infrastructure already has a p4c backend this is adding another P4
>    backend instead of getting the rather small group of people to work on
>    a single backend we are now creating another one.
>
>  - Common reasons I think would justify a new P4 backend and implementation
>    would be: speed efficiency, or expressiveness. I think this
>    implementation is neither more efficient nor more expressive. Concrete
>    examples on expressiveness would be interesting, but I don't see any.
>    Loops were mentioned once but latest kernels have loop support.
>
>  - The main talking point for many slide decks about p4tc is hardware
>    offload. This seems like the main benefit of pushing the P4 DSL into the
>    kernel. But, we have no hw implementation, not even a vendor stepping up
>    to comment on this implementation and how it will work for them. HW
>    introduces all sorts of interesting problems that I don't see how we
>    solve in this framework. For example a few off the top of my head:
>    syncing current state into tc, how does operator program tc inside
>    constraints, who writes the p4 models for these hardware devices, do
>    they fit into this 'tc' infrastructure, partial updates into hardware
>    seems unlikely to work for most hardware, ...
>
>  - The kfuncs are mostly duplicates of map ops we already have in BPF API.
>    The motivation by my read is to use netlink instead of bpf commands. I
>    don't agree with this, optimizing for some low level debug a developer
>    uses is the wrong design space. Actual users should not be deploying
>    this via ssh into boxes. The workflow will not scale and really we need
>    tooling and infra to land P4 programs across the network. This is orders
>    of more pain if its an endpoint solution and not a middlebox/switch
>    solution. As a switch solution I don't see how p4tc sw scales to even TOR
>    packet rates. So you need tooling on top and user interact with the
>    tooling not the Linux widget/debugger at the bottom.
>
>  - There is no performance analysis: The comment was functionality before
>    performance which I disagree with. If it was a first implementation and
>    we didn't have a way to do P4 DSL already than I might agree, but here
>    we have an existing solution so it should be at least as good and should
>    be better than existing backend. A software datapath adoption is going
>    to be critically based on performance. I don't see taking even a 5% hit
>    when porting over to P4 from existing datapath.
>
> Commentary: I think its 100% correct to debate how the P4 DSL is
> implemented in the kernel. I can't see why this is off limits somehow this
> patch set proposes an approach there could be many approaches. BPF comes up
> not because I'm some BPF zealot that needs P4 DSL in BPF, but because it
> exists today there is even a P4 backend. Fundamentally I don't see the
> value add we get by creating two P4 pipelines this is going to create
> duplication all the way up to the P4 tooling/infra through to the kernel.
> From your side you keep saying I'm bike shedding and demanding BPF, but
> from my perspective your introducing another entire toolchain simply
> because you want some low level debug commands that 99% of P4 users should
> not be using or caring about.
>
> To try and be constructive some things that would change my mind would
> be a vendor showing how hardware can be used. This would be compelling.
> Or performance showing its somehow gets a more performant implementation.
> Or lastly if the current p4c implementation is fundamentally broken
> somehow.
>

John,
With all due respect we are going back again over the same points,
recycled many times over to which i have responded to you many times.
It's gettting tiring.  This is exactly why i called it bikeshedding.
Let's just agree to disagree.

cheers,
jamal

> Thanks
> John

  reply	other threads:[~2024-02-28 18:23 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-25 16:54 [PATCH net-next v12 00/15] Introducing P4TC (series 1) Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 01/15] net: sched: act_api: Introduce P4 actions list Jamal Hadi Salim
2024-02-29 15:05   ` Paolo Abeni
2024-02-29 18:21     ` Jamal Hadi Salim
2024-03-01  7:30       ` Paolo Abeni
2024-03-01 12:39         ` Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 02/15] net/sched: act_api: increase action kind string length Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 03/15] net/sched: act_api: Update tc_action_ops to account for P4 actions Jamal Hadi Salim
2024-02-29 16:19   ` Paolo Abeni
2024-02-29 18:30     ` Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 04/15] net/sched: act_api: add struct p4tc_action_ops as a parameter to lookup callback Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 05/15] net: sched: act_api: Add support for preallocated P4 action instances Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 06/15] p4tc: add P4 data types Jamal Hadi Salim
2024-02-29 15:09   ` Paolo Abeni
2024-02-29 18:31     ` Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 07/15] p4tc: add template API Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 08/15] p4tc: add template pipeline create, get, update, delete Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 09/15] p4tc: add template action create, update, delete, get, flush and dump Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 10/15] p4tc: add runtime action support Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 11/15] p4tc: add template table create, update, delete, get, flush and dump Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 12/15] p4tc: add runtime table entry create and update Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 13/15] p4tc: add runtime table entry get, delete, flush and dump Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 14/15] p4tc: add set of P4TC table kfuncs Jamal Hadi Salim
2024-03-01  6:53   ` Martin KaFai Lau
2024-03-01 12:31     ` Jamal Hadi Salim
2024-03-03  1:32       ` Martin KaFai Lau
2024-03-03 17:20         ` Jamal Hadi Salim
2024-03-05  7:40           ` Martin KaFai Lau
2024-03-05 12:30             ` Jamal Hadi Salim
2024-03-06  7:58               ` Martin KaFai Lau
2024-03-06 20:22                 ` Jamal Hadi Salim
2024-03-06 22:21                   ` Martin KaFai Lau
2024-03-06 23:19                     ` Jamal Hadi Salim
2024-02-25 16:54 ` [PATCH net-next v12 15/15] p4tc: add P4 classifier Jamal Hadi Salim
2024-02-28 17:11 ` [PATCH net-next v12 00/15] Introducing P4TC (series 1) John Fastabend
2024-02-28 18:23   ` Jamal Hadi Salim [this message]
2024-02-28 21:13     ` John Fastabend
2024-03-01  7:02   ` Martin KaFai Lau
2024-03-01 12:36     ` Jamal Hadi Salim
2024-02-29 17:13 ` Paolo Abeni
2024-02-29 18:49   ` Jamal Hadi Salim
2024-02-29 20:52     ` John Fastabend
2024-02-29 21:49   ` Singhai, Anjali
2024-02-29 22:33     ` John Fastabend
2024-02-29 22:48       ` Jamal Hadi Salim
     [not found]         ` <CAOuuhY8qbsYCjdUYUZv8J3jz8HGXmtxLmTDP6LKgN5uRVZwMnQ@mail.gmail.com>
2024-03-01 17:00           ` Jakub Kicinski
2024-03-01 17:39             ` Jamal Hadi Salim
2024-03-02  1:32               ` Jakub Kicinski
2024-03-02  2:20                 ` Tom Herbert
2024-03-03  3:15                   ` Jakub Kicinski
2024-03-03 16:31                     ` Tom Herbert
2024-03-04 20:07                       ` Jakub Kicinski
2024-03-04 20:58                         ` eBPF to implement core functionility WAS " Tom Herbert
2024-03-04 21:19                       ` Stanislav Fomichev
2024-03-04 22:01                         ` Tom Herbert
2024-03-04 23:24                           ` Stanislav Fomichev
2024-03-04 23:50                             ` Tom Herbert
2024-03-02  2:59                 ` Hardware Offload discussion WAS(Re: " Jamal Hadi Salim
2024-03-02 14:36                   ` Jamal Hadi Salim
2024-03-03  3:27                     ` Jakub Kicinski
2024-03-03 17:00                       ` Jamal Hadi Salim
2024-03-03 18:10                         ` Tom Herbert
2024-03-03 19:04                           ` Jamal Hadi Salim
2024-03-04 20:18                             ` Jakub Kicinski
2024-03-04 21:02                               ` Jamal Hadi Salim
2024-03-04 21:23                             ` Stanislav Fomichev
2024-03-04 21:44                               ` Jamal Hadi Salim
2024-03-04 22:23                                 ` Stanislav Fomichev
2024-03-04 22:59                                   ` Jamal Hadi Salim
2024-03-04 23:14                                     ` Stanislav Fomichev
2024-03-01 18:53   ` Chris Sommers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAM0EoMnMrOAZ1iGocDDhVmoeY33fxZjiUEQc4yp0KJj8nASrAA@mail.gmail.com \
    --to=jhs@mojatatu.com \
    --cc=Mahesh.Shirshyad@amd.com \
    --cc=Vipin.Jain@amd.com \
    --cc=andy.fingerhut@gmail.com \
    --cc=anjali.singhai@intel.com \
    --cc=bpf@vger.kernel.org \
    --cc=chris.sommers@keysight.com \
    --cc=dan.daly@intel.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=deb.chatterjee@intel.com \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=jiri@resnulli.us \
    --cc=john.fastabend@gmail.com \
    --cc=khalidm@nvidia.com \
    --cc=kuba@kernel.org \
    --cc=mattyk@nvidia.com \
    --cc=mleitner@redhat.com \
    --cc=namrata.limaye@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=pctammela@mojatatu.com \
    --cc=toke@redhat.com \
    --cc=tom@sipanda.io \
    --cc=tomasz.osinski@intel.com \
    --cc=victor@mojatatu.com \
    --cc=vladbu@nvidia.com \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).