Message-ID: <ZVuIX06VKvrsmm1S@nanopsycho>
Date: Mon, 20 Nov 2023 17:25:03 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jamal Hadi Salim <jhs@...atatu.com>
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

Mon, Nov 20, 2023 at 04:30:11PM CET, jhs@...atatu.com wrote:
>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.

That or use genetlink to have per-cmd attributes.


>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?

Nope, you can run ynl.py and let it parse the yaml on fly.


>
>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