Message-ID: <CAM0EoM=EFJTqeEsJHQZw-3x6TnEMFYT1+Rsm7f4aSKh0QLqBnA@mail.gmail.com>
Date: Mon, 20 Nov 2023 10:30:11 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, deb.chatterjee@...el.com, anjali.singhai@...el.com, 
	namrata.limaye@...el.com, tom@...anda.io, mleitner@...hat.com, 
	Mahesh.Shirshyad@....com, tomasz.osinski@...el.com, xiyou.wangcong@...il.com, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	vladbu@...dia.com, horms@...nel.org, daniel@...earbox.net, 
	bpf@...r.kernel.org, khalidm@...dia.com, toke@...hat.com, mattyk@...dia.com, 
	David Ahern <dsahern@...il.com>, Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH net-next v8 09/15] p4tc: add template pipeline create,
 get, update, delete

On Mon, Nov 20, 2023 at 8:16 AM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Mon, Nov 20, 2023 at 01:48:14PM CET, jhs@...atatu.com wrote:
> >On Mon, Nov 20, 2023 at 3:18 AM Jiri Pirko <jiri@...nulli.us> wrote:
> >>
> >> Fri, Nov 17, 2023 at 01:09:45PM CET, jhs@...atatu.com wrote:
> >> >On Thu, Nov 16, 2023 at 11:11 AM Jiri Pirko <jiri@...nulli.us> wrote:
> >> >>
> >> >> Thu, Nov 16, 2023 at 03:59:42PM CET, jhs@...atatu.com wrote:
> >> >>
> >> >> [...]
> >> >>
> >> >>
> >> >> >diff --git a/include/uapi/linux/p4tc.h b/include/uapi/linux/p4tc.h
> >> >> >index ba32dba66..4d33f44c1 100644
> >> >> >--- a/include/uapi/linux/p4tc.h
> >> >> >+++ b/include/uapi/linux/p4tc.h
> >> >> >@@ -2,8 +2,71 @@
> >> >> > #ifndef __LINUX_P4TC_H
> >> >> > #define __LINUX_P4TC_H
> >> >> >
> >> >> >+#include <linux/types.h>
> >> >> >+#include <linux/pkt_sched.h>
> >> >> >+
> >> >> >+/* pipeline header */
> >> >> >+struct p4tcmsg {
> >> >> >+      __u32 pipeid;
> >> >> >+      __u32 obj;
> >> >> >+};
> >> >>
> >> >> I don't follow. Is there any sane reason to use header instead of normal
> >> >> netlink attribute? Moveover, you extend the existing RT netlink with
> >> >> a huge amout of p4 things. Isn't this the good time to finally introduce
> >> >> generic netlink TC family with proper yaml spec with all the benefits it
> >> >> brings and implement p4 tc uapi there? Please?
> >> >>
> >> >
> >> >Several reasons:
> >> >a) We are similar to current tc messaging with the subheader being
> >> >there for multiplexing.
> >>
> >> Yeah, you don't need to carry 20year old burden in newly introduced
> >> interface. That's my point.
> >
> >Having a demux sub header is 20 year old burden? I didnt follow.
>
> You don't need the header, that's my point.
>

Let me see if i understand you:
We have multiple object types per pipeline - this info is _omni
present and it is never going to change_.
Your view is, have a hierarchy of attributes and put this subheader in
probably one attribute at the root.
You parse the root, you find the obj and pipeid and then you use that
to parse the rest of the per-object specific
attributes?

I dont know if a hierarchical attribute layout gives you any advantage
over the subheader approach - unless we figure a way to annotate
attributes as "optional" vs "must be present". I agree that getting
the validation for free is a bonus ..


> >
> >>
> >> >b) Where does this leave iproute2? +Cc David and Stephen. Do other
> >> >generic netlink conversions get contributed back to iproute2?
> >>
> >> There is no conversion afaik, only extensions. And they has to be,
> >> otherwise the user would not be able to use the newly introduced
> >> features.
> >
> >The big question is does the collective who use iproute2 still get to
> >use the same tooling or now they have to go and learn some new
> >tooling. I understand the value of the new approach but is it a
> >revolution or an evolution? We opted to put thing in iproute2 instead
> >for example because that is widely available (and used).
>
> I don't see why iproute2 user facing interface would be any different
> depending on if you user RTnetlink or genetlink as backend channel...
>

iproute2 supports plenty of genetlink already.
We need to find a way to have the best of both worlds.

>
> >
> >>
> >> >c) note: Our API is CRUD-ish instead of RPC(per generic netlink)
> >> >based. i.e you have:
> >> > COMMAND <PATH/TO/OBJECT> [optional data]  so we can support arbitrary
> >> >P4 programs from the control plane.
> >>
> >> I'm pretty sure you can achieve the same over genetlink.
> >>
> >
> >I think you are right.
> >
> >>
> >> >d) we have spent many hours optimizing the control to the kernel so i
> >> >am not sure what it would buy us to switch to generic netlink..
> >>
> >> All the benefits of ynl yaml tooling, at least.
> >>
> >
> >Did you pay close attention to what we have? The user space code is
> >written once into iproute2 and subsequent to that there is no
> >recompilation  of any iproute2 code. The compiler generates a json
> >file specific to a P4 program which is then introspected by the
> >iproute2 code.
>
> Right, but in real life, netlink is used directly by many apps. I don't
> see why this is any different.
>

Not sure if you were referring to what i said about the json file or
something else. The main value is not just kernel independence but
also iproute2 independence i.e not need to compile any code.

> Plus, the very best part of yaml from user perpective I see is,
> you just need the kernel-git yaml file and you can submit all commands.
> No userspace implementation needed.

Two different tacts: i can see this as being developer friendly (and
we are more trying to be operator friendly).
I need to take a closer look. Sounds like it should be polyglot
friendly as well. If i am not mistaken you still have to compile code
as a result of generation from the yaml?

cheers,
jamal

>
> >
> >
> >cheers,
> >jamal
> >
> >>
> >> >
> >> >cheers,
> >> >jamal
> >> >
> >> >>
> >> >> >+
> >> >> >+#define P4TC_MAXPIPELINE_COUNT 32
> >> >> >+#define P4TC_MAXTABLES_COUNT 32
> >> >> >+#define P4TC_MINTABLES_COUNT 0
> >> >> >+#define P4TC_MSGBATCH_SIZE 16
> >> >> >+
> >> >> > #define P4TC_MAX_KEYSZ 512
> >> >> >
> >> >> >+#define TEMPLATENAMSZ 32
> >> >> >+#define PIPELINENAMSIZ TEMPLATENAMSZ
> >> >>
> >> >> ugh. A prefix please?
> >> >>
> >> >> pw-bot: cr
> >> >>
> >> >> [...]

Powered by blists - more mailing lists