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

[Jiri Pirko <jiri@resnulli.us>]

Mon, Nov 20, 2023 at 03:02:49PM CET, jhs@mojatatu.com wrote:
>On Mon, Nov 20, 2023 at 3:22 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> 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.
>>
>
>Sorry, still not following:
>Lets say i split the current patchset 1 with patch 1-8 (which are
>trivial and have been reviewed) then make the rest into patchset 2
>with a new set 1-8. I dont see how patchset 2 compiles unless it has
>access to code from patchset 1. Unless patchset 1 is merged i dont see
>how this works with patchwork or reviewers. Am i missing something?

Why it would not work. Describe your motivation and plans and submit
part of the work, the rest later on. No problem.


>
>cheers,
>jamal
>
>>
>> >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
>> >
>> >> [...]