Re: [PATCH net-next v8 15/15] p4tc: Add P4 extern interface

Jiri Pirko
2023-11-20T08:22:05Z

Fri, Nov 17, 2023 at 01:14:43PM CET, jhs@mojatatu.com wrote:
>On Thu, Nov 16, 2023 at 11:42 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Nov 16, 2023 at 03:59:48PM CET, jhs@mojatatu.com wrote:
>>
>> [...]
>>
>> > include/net/p4tc.h | 161 +++
>> > include/net/p4tc_ext_api.h | 199 +++
>> > include/uapi/linux/p4tc.h | 61 +
>> > include/uapi/linux/p4tc_ext.h | 36 +
>> > net/sched/p4tc/Makefile | 2 +-
>> > net/sched/p4tc/p4tc_bpf.c | 79 +-
>> > net/sched/p4tc/p4tc_ext.c | 2204 ++++++++++++++++++++++++++++
>> > net/sched/p4tc/p4tc_pipeline.c | 34 +-
>> > net/sched/p4tc/p4tc_runtime_api.c | 10 +-
>> > net/sched/p4tc/p4tc_table.c | 57 +-
>> > net/sched/p4tc/p4tc_tbl_entry.c | 25 +-
>> > net/sched/p4tc/p4tc_tmpl_api.c | 4 +
>> > net/sched/p4tc/p4tc_tmpl_ext.c | 2221 +++++++++++++++++++++++++++++
>> > 13 files changed, 5083 insertions(+), 10 deletions(-)
>>
>> This is for this patch. Now for the whole patchset you have:
>> 30 files changed, 16676 insertions(+), 39 deletions(-)
>>
>> I understand that you want to fit into 15 patches with all the work.
>> But sorry, patches like this are unreviewable. My suggestion is to split
>> the patchset into multiple ones including smaller patches and allow
>> people to digest this. I don't believe that anyone can seriously stand
>> to review a patch with more than 200 lines changes.
>
>This specific patch is not difficult to split into two. I can do that
>and send out minus the first 8 trivial patches - but not familiar with
>how to do "here's part 1 of the patches" and "here's patchset two".

Split into multiple patchsets and send one by one. No need to have all
in at once.

>There's dependency between them so not clear how patchwork and

What dependency. It should compile. Introduce some basic functionality
first and extend it incrementally with other patchsets. The usual way.

>reviewers would deal with it. Thoughts?
>
>Note: The code machinery is really repeatable; for example if you look
>at the tables control you will see very similar patterns to actions
>etc. i.e spending time to review one will make it easy for the rest.
>
>cheers,
>jamal
>
>> …