Re: [PATCH net-next v8 09/15] p4tc: add template pipeline create, get, update, delete

[Jiri Pirko <jiri@resnulli.us>]

Mon, Nov 20, 2023 at 01:48:14PM CET, jhs@mojatatu.com wrote:
>On Mon, Nov 20, 2023 at 3:18 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Fri, Nov 17, 2023 at 01:09:45PM CET, jhs@mojatatu.com wrote:
>> >On Thu, Nov 16, 2023 at 11:11 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Thu, Nov 16, 2023 at 03:59:42PM CET, jhs@mojatatu.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.


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


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

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.


>
>
>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
>> >>
>> >> [...]