mbox series

[v4,00/41] Pipeline alignment with the P4 language

Message ID 20200910152645.9342-1-cristian.dumitrescu@intel.com (mailing list archive)
Headers
Series Pipeline alignment with the P4 language |

Message

Cristian Dumitrescu Sept. 10, 2020, 3:26 p.m. UTC 
  This patch set introduces a new pipeline type that combines the DPDK
performance with the flexibility of the P4-16 language[1]. The new API
can be used either by itself to code a complete software switch (SWX)
or data plane app, or in combination with the open-source P4 compiler
P4C [2], potentially acting as a P4C back-end, thus allowing the P4
programs to be translated to the DPDK API and run on multi-core CPUs.

Main new features:

* Nothing is hard-wired, everything is dynamically defined: The packet
  headers (i.e. protocols), the packet meta-data, the actions, the
  tables and the pipeline itself are dynamically defined instead of
  having to be selected from a pre-defined set.

* Instructions: The actions and the life of the packet through the
  pipeline are defined with instructions that manipulate the pipeline
  objects mentioned above. The pipeline is the main function of the
  packet program, with actions as subroutines triggered by the tables.

* Call external plugins: Extern objects and functions can be defined
  to call functionality that cannot be efficiently implemented with
  the existing pipeline-oriented instruction set, such as: special
  error detecting/correcting codes, crypto, meters, stats arrays,
  heuristics, etc.

* Better control plane interaction: Transaction-oriented table update
  mechanism that supports multi-table atomic updates. Multiple tables
  can be updated in a single step with only the before and after table
  sets visible to the packets. Alignment with P4Runtime [3].

* Performance: Multiple packets are in-flight within the pipeline at
  any moment. Each packet is owned by a different time-sharing thread
  in run-to-completion, with the thread pausing before memory access
  operations such as packet I/O and table lookup to allow the memory
  prefetch to complete. The instructions are verified and translated
  at initialization time with no run-time impact. The instructions are
  also optimized to detect and "fuse" frequently used patterns into
  vector-like instructions transparently to the user.

API deprecation and maturing roadmap:
* The existing pipeline stable API (rte_pipeline.h) to be deprecated
  prior to and removed as part of the DPDK 21.11 LTS release. 
* The new SWX pipeline experimental API (rte_swx_pipeline.h) to mature
  and become stable as part of the same DPDK 21.11 LTS release.

V4 changes:
* Spell check fixes.

V3 changes:
* Removed the library Makefile support to align with the latest DPDK.

V2 changes:
* Updated the title and commit messages to reflect the introduction of
  the new SWX pipeline type.
* Added the API deprecation and maturing roadmap to the cover letter.
* Added support for building the SWX pipeline based on specification
  file with syntax aligned to the P4 language. The spec file may be
  generated by the P4C compiler in the future (see patch 32). Reworked
  the examples accordingly (see patches 39, 40 and 41).
* Added support for the SWX sink port (used for packet drop or log)
  when PCAP library is disabled from the build.
* Added checks to the application CLI commands to prevent execution
  when dependencies of the current command have previously failed (see
  patch 38).
* Fixed build warning for 32-bit targets due to the printing of 64-bit
  statistics counters (see patch 38).

[1] P4-16 specification: https://p4.org/p4-spec/docs/P4-16-v1.2.1.pdf
[2] P4-16 compiler: https://github.com/p4lang/p4c
[3] P4Runtime specification:
    https://p4.org/p4runtime/spec/v1.2.0/P4Runtime-Spec.pdf

Cristian Dumitrescu (41):
  pipeline: add new SWX pipeline type
  pipeline: add SWX pipeline input port
  pipeline: add SWX pipeline output port
  pipeline: add SWX headers and meta-data
  pipeline: add SWX extern objects and funcs
  pipeline: add SWX pipeline action
  pipeline: add SWX pipeline tables
  pipeline: add SWX pipeline instructions
  pipeline: add SWX rx and extract instructions
  pipeline: add SWX tx and emit instructions
  pipeline: add header validate and invalidate SWX instructions
  pipeline: add SWX mov instruction
  pipeline: add SWX dma instruction
  pipeline: introduce SWX add instruction
  pipeline: introduce SWX sub instruction
  pipeline: introduce SWX ckadd instruction
  pipeline: introduce SWX cksub instruction
  pipeline: introduce SWX and instruction
  pipeline: introduce SWX or instruction
  pipeline: introduce SWX xor instruction
  pipeline: introduce SWX shl instruction
  pipeline: introduce SWX shr instruction
  pipeline: introduce SWX table instruction
  pipeline: introduce SWX extern instruction
  pipeline: introduce SWX jmp and return instructions
  pipeline: add SWX instruction description
  pipeline: add SWX instruction verifier
  pipeline: add SWX instruction optimizer
  pipeline: add SWX pipeline query API
  pipeline: add SWX pipeline flush
  pipeline: add SWX table update high level API
  pipeline: add SWX pipeline specification file
  port: add ethernet device SWX port
  port: add source and sink SWX ports
  table: add exact match SWX table
  examples/pipeline: add new example application
  examples/pipeline: add message passing mechanism
  examples/pipeline: add configuration commands
  examples/pipeline: add l2fwd example
  examples/pipeline: add l2fwd with MAC swap example
  examples/pipeline: add VXLAN encapsulation example

 examples/meson.build                          |    1 +
 examples/pipeline/Makefile                    |   53 +
 examples/pipeline/cli.c                       | 1400 ++++
 examples/pipeline/cli.h                       |   19 +
 examples/pipeline/conn.c                      |  331 +
 examples/pipeline/conn.h                      |   50 +
 examples/pipeline/examples/l2fwd.cli          |   25 +
 examples/pipeline/examples/l2fwd.spec         |   42 +
 examples/pipeline/examples/l2fwd_macswp.cli   |   25 +
 examples/pipeline/examples/l2fwd_macswp.spec  |   59 +
 .../pipeline/examples/l2fwd_macswp_pcap.cli   |   20 +
 examples/pipeline/examples/l2fwd_pcap.cli     |   20 +
 examples/pipeline/examples/packet.txt         |  102 +
 examples/pipeline/examples/vxlan.cli          |   27 +
 examples/pipeline/examples/vxlan.spec         |  173 +
 examples/pipeline/examples/vxlan_pcap.cli     |   22 +
 examples/pipeline/examples/vxlan_table.py     |   71 +
 examples/pipeline/examples/vxlan_table.txt    |   16 +
 examples/pipeline/main.c                      |  193 +
 examples/pipeline/meson.build                 |   18 +
 examples/pipeline/obj.c                       |  470 ++
 examples/pipeline/obj.h                       |  131 +
 examples/pipeline/thread.c                    |  549 ++
 examples/pipeline/thread.h                    |   28 +
 lib/librte_pipeline/meson.build               |   14 +-
 lib/librte_pipeline/rte_pipeline_version.map  |   44 +-
 lib/librte_pipeline/rte_swx_ctl.c             | 1552 ++++
 lib/librte_pipeline/rte_swx_ctl.h             |  568 ++
 lib/librte_pipeline/rte_swx_extern.h          |   98 +
 lib/librte_pipeline/rte_swx_pipeline.c        | 7197 +++++++++++++++++
 lib/librte_pipeline/rte_swx_pipeline.h        |  711 ++
 lib/librte_pipeline/rte_swx_pipeline_spec.c   | 1439 ++++
 lib/librte_port/meson.build                   |    9 +-
 lib/librte_port/rte_port_version.map          |    5 +-
 lib/librte_port/rte_swx_port.h                |  202 +
 lib/librte_port/rte_swx_port_ethdev.c         |  313 +
 lib/librte_port/rte_swx_port_ethdev.h         |   54 +
 lib/librte_port/rte_swx_port_source_sink.c    |  335 +
 lib/librte_port/rte_swx_port_source_sink.h    |   57 +
 lib/librte_table/meson.build                  |    7 +-
 lib/librte_table/rte_swx_table.h              |  295 +
 lib/librte_table/rte_swx_table_em.c           |  851 ++
 lib/librte_table/rte_swx_table_em.h           |   30 +
 lib/librte_table/rte_table_version.map        |    7 +
 44 files changed, 17625 insertions(+), 8 deletions(-)
 create mode 100644 examples/pipeline/Makefile
 create mode 100644 examples/pipeline/cli.c
 create mode 100644 examples/pipeline/cli.h
 create mode 100644 examples/pipeline/conn.c
 create mode 100644 examples/pipeline/conn.h
 create mode 100644 examples/pipeline/examples/l2fwd.cli
 create mode 100644 examples/pipeline/examples/l2fwd.spec
 create mode 100644 examples/pipeline/examples/l2fwd_macswp.cli
 create mode 100644 examples/pipeline/examples/l2fwd_macswp.spec
 create mode 100644 examples/pipeline/examples/l2fwd_macswp_pcap.cli
 create mode 100644 examples/pipeline/examples/l2fwd_pcap.cli
 create mode 100644 examples/pipeline/examples/packet.txt
 create mode 100644 examples/pipeline/examples/vxlan.cli
 create mode 100644 examples/pipeline/examples/vxlan.spec
 create mode 100644 examples/pipeline/examples/vxlan_pcap.cli
 create mode 100644 examples/pipeline/examples/vxlan_table.py
 create mode 100644 examples/pipeline/examples/vxlan_table.txt
 create mode 100644 examples/pipeline/main.c
 create mode 100644 examples/pipeline/meson.build
 create mode 100644 examples/pipeline/obj.c
 create mode 100644 examples/pipeline/obj.h
 create mode 100644 examples/pipeline/thread.c
 create mode 100644 examples/pipeline/thread.h
 create mode 100644 lib/librte_pipeline/rte_swx_ctl.c
 create mode 100644 lib/librte_pipeline/rte_swx_ctl.h
 create mode 100644 lib/librte_pipeline/rte_swx_extern.h
 create mode 100644 lib/librte_pipeline/rte_swx_pipeline.c
 create mode 100644 lib/librte_pipeline/rte_swx_pipeline.h
 create mode 100644 lib/librte_pipeline/rte_swx_pipeline_spec.c
 create mode 100644 lib/librte_port/rte_swx_port.h
 create mode 100644 lib/librte_port/rte_swx_port_ethdev.c
 create mode 100644 lib/librte_port/rte_swx_port_ethdev.h
 create mode 100644 lib/librte_port/rte_swx_port_source_sink.c
 create mode 100644 lib/librte_port/rte_swx_port_source_sink.h
 create mode 100644 lib/librte_table/rte_swx_table.h
 create mode 100644 lib/librte_table/rte_swx_table_em.c
 create mode 100644 lib/librte_table/rte_swx_table_em.h

Comments

Wang, Han2 Sept. 22, 2020, 8:05 p.m. UTC | #1 
Cristian

Thanks for updating the patch set. As I mentioned in my previous email, we are working on adding support for this as a new P4C CPU target. We plan to open source the compiler backend in the near future.

Cheers,
Han

On 9/10/20, 8:27 AM, "dev on behalf of Cristian Dumitrescu" <dev-bounces@dpdk.org on behalf of cristian.dumitrescu@intel.com> wrote:

    This patch set introduces a new pipeline type that combines the DPDK
    performance with the flexibility of the P4-16 language[1]. The new API
    can be used either by itself to code a complete software switch (SWX)
    or data plane app, or in combination with the open-source P4 compiler
    P4C [2], potentially acting as a P4C back-end, thus allowing the P4
    programs to be translated to the DPDK API and run on multi-core CPUs.

    Main new features:

    * Nothing is hard-wired, everything is dynamically defined: The packet
      headers (i.e. protocols), the packet meta-data, the actions, the
      tables and the pipeline itself are dynamically defined instead of
      having to be selected from a pre-defined set.

    * Instructions: The actions and the life of the packet through the
      pipeline are defined with instructions that manipulate the pipeline
      objects mentioned above. The pipeline is the main function of the
      packet program, with actions as subroutines triggered by the tables.

    * Call external plugins: Extern objects and functions can be defined
      to call functionality that cannot be efficiently implemented with
      the existing pipeline-oriented instruction set, such as: special
      error detecting/correcting codes, crypto, meters, stats arrays,
      heuristics, etc.

    * Better control plane interaction: Transaction-oriented table update
      mechanism that supports multi-table atomic updates. Multiple tables
      can be updated in a single step with only the before and after table
      sets visible to the packets. Alignment with P4Runtime [3].

    * Performance: Multiple packets are in-flight within the pipeline at
      any moment. Each packet is owned by a different time-sharing thread
      in run-to-completion, with the thread pausing before memory access
      operations such as packet I/O and table lookup to allow the memory
      prefetch to complete. The instructions are verified and translated
      at initialization time with no run-time impact. The instructions are
      also optimized to detect and "fuse" frequently used patterns into
      vector-like instructions transparently to the user.

    API deprecation and maturing roadmap:
    * The existing pipeline stable API (rte_pipeline.h) to be deprecated
      prior to and removed as part of the DPDK 21.11 LTS release. 
    * The new SWX pipeline experimental API (rte_swx_pipeline.h) to mature
      and become stable as part of the same DPDK 21.11 LTS release.

    V4 changes:
    * Spell check fixes.

    V3 changes:
    * Removed the library Makefile support to align with the latest DPDK.

    V2 changes:
    * Updated the title and commit messages to reflect the introduction of
      the new SWX pipeline type.
    * Added the API deprecation and maturing roadmap to the cover letter.
    * Added support for building the SWX pipeline based on specification
      file with syntax aligned to the P4 language. The spec file may be
      generated by the P4C compiler in the future (see patch 32). Reworked
      the examples accordingly (see patches 39, 40 and 41).
    * Added support for the SWX sink port (used for packet drop or log)
      when PCAP library is disabled from the build.
    * Added checks to the application CLI commands to prevent execution
      when dependencies of the current command have previously failed (see
      patch 38).
    * Fixed build warning for 32-bit targets due to the printing of 64-bit
      statistics counters (see patch 38).

    [1] P4-16 specification: https://p4.org/p4-spec/docs/P4-16-v1.2.1.pdf
    [2] P4-16 compiler: https://github.com/p4lang/p4c
    [3] P4Runtime specification:
        https://p4.org/p4runtime/spec/v1.2.0/P4Runtime-Spec.pdf

    Cristian Dumitrescu (41):
      pipeline: add new SWX pipeline type
      pipeline: add SWX pipeline input port
      pipeline: add SWX pipeline output port
      pipeline: add SWX headers and meta-data
      pipeline: add SWX extern objects and funcs
      pipeline: add SWX pipeline action
      pipeline: add SWX pipeline tables
      pipeline: add SWX pipeline instructions
      pipeline: add SWX rx and extract instructions
      pipeline: add SWX tx and emit instructions
      pipeline: add header validate and invalidate SWX instructions
      pipeline: add SWX mov instruction
      pipeline: add SWX dma instruction
      pipeline: introduce SWX add instruction
      pipeline: introduce SWX sub instruction
      pipeline: introduce SWX ckadd instruction
      pipeline: introduce SWX cksub instruction
      pipeline: introduce SWX and instruction
      pipeline: introduce SWX or instruction
      pipeline: introduce SWX xor instruction
      pipeline: introduce SWX shl instruction
      pipeline: introduce SWX shr instruction
      pipeline: introduce SWX table instruction
      pipeline: introduce SWX extern instruction
      pipeline: introduce SWX jmp and return instructions
      pipeline: add SWX instruction description
      pipeline: add SWX instruction verifier
      pipeline: add SWX instruction optimizer
      pipeline: add SWX pipeline query API
      pipeline: add SWX pipeline flush
      pipeline: add SWX table update high level API
      pipeline: add SWX pipeline specification file
      port: add ethernet device SWX port
      port: add source and sink SWX ports
      table: add exact match SWX table
      examples/pipeline: add new example application
      examples/pipeline: add message passing mechanism
      examples/pipeline: add configuration commands
      examples/pipeline: add l2fwd example
      examples/pipeline: add l2fwd with MAC swap example
      examples/pipeline: add VXLAN encapsulation example

     examples/meson.build                          |    1 +
     examples/pipeline/Makefile                    |   53 +
     examples/pipeline/cli.c                       | 1400 ++++
     examples/pipeline/cli.h                       |   19 +
     examples/pipeline/conn.c                      |  331 +
     examples/pipeline/conn.h                      |   50 +
     examples/pipeline/examples/l2fwd.cli          |   25 +
     examples/pipeline/examples/l2fwd.spec         |   42 +
     examples/pipeline/examples/l2fwd_macswp.cli   |   25 +
     examples/pipeline/examples/l2fwd_macswp.spec  |   59 +
     .../pipeline/examples/l2fwd_macswp_pcap.cli   |   20 +
     examples/pipeline/examples/l2fwd_pcap.cli     |   20 +
     examples/pipeline/examples/packet.txt         |  102 +
     examples/pipeline/examples/vxlan.cli          |   27 +
     examples/pipeline/examples/vxlan.spec         |  173 +
     examples/pipeline/examples/vxlan_pcap.cli     |   22 +
     examples/pipeline/examples/vxlan_table.py     |   71 +
     examples/pipeline/examples/vxlan_table.txt    |   16 +
     examples/pipeline/main.c                      |  193 +
     examples/pipeline/meson.build                 |   18 +
     examples/pipeline/obj.c                       |  470 ++
     examples/pipeline/obj.h                       |  131 +
     examples/pipeline/thread.c                    |  549 ++
     examples/pipeline/thread.h                    |   28 +
     lib/librte_pipeline/meson.build               |   14 +-
     lib/librte_pipeline/rte_pipeline_version.map  |   44 +-
     lib/librte_pipeline/rte_swx_ctl.c             | 1552 ++++
     lib/librte_pipeline/rte_swx_ctl.h             |  568 ++
     lib/librte_pipeline/rte_swx_extern.h          |   98 +
     lib/librte_pipeline/rte_swx_pipeline.c        | 7197 +++++++++++++++++
     lib/librte_pipeline/rte_swx_pipeline.h        |  711 ++
     lib/librte_pipeline/rte_swx_pipeline_spec.c   | 1439 ++++
     lib/librte_port/meson.build                   |    9 +-
     lib/librte_port/rte_port_version.map          |    5 +-
     lib/librte_port/rte_swx_port.h                |  202 +
     lib/librte_port/rte_swx_port_ethdev.c         |  313 +
     lib/librte_port/rte_swx_port_ethdev.h         |   54 +
     lib/librte_port/rte_swx_port_source_sink.c    |  335 +
     lib/librte_port/rte_swx_port_source_sink.h    |   57 +
     lib/librte_table/meson.build                  |    7 +-
     lib/librte_table/rte_swx_table.h              |  295 +
     lib/librte_table/rte_swx_table_em.c           |  851 ++
     lib/librte_table/rte_swx_table_em.h           |   30 +
     lib/librte_table/rte_table_version.map        |    7 +
     44 files changed, 17625 insertions(+), 8 deletions(-)
     create mode 100644 examples/pipeline/Makefile
     create mode 100644 examples/pipeline/cli.c
     create mode 100644 examples/pipeline/cli.h
     create mode 100644 examples/pipeline/conn.c
     create mode 100644 examples/pipeline/conn.h
     create mode 100644 examples/pipeline/examples/l2fwd.cli
     create mode 100644 examples/pipeline/examples/l2fwd.spec
     create mode 100644 examples/pipeline/examples/l2fwd_macswp.cli
     create mode 100644 examples/pipeline/examples/l2fwd_macswp.spec
     create mode 100644 examples/pipeline/examples/l2fwd_macswp_pcap.cli
     create mode 100644 examples/pipeline/examples/l2fwd_pcap.cli
     create mode 100644 examples/pipeline/examples/packet.txt
     create mode 100644 examples/pipeline/examples/vxlan.cli
     create mode 100644 examples/pipeline/examples/vxlan.spec
     create mode 100644 examples/pipeline/examples/vxlan_pcap.cli
     create mode 100644 examples/pipeline/examples/vxlan_table.py
     create mode 100644 examples/pipeline/examples/vxlan_table.txt
     create mode 100644 examples/pipeline/main.c
     create mode 100644 examples/pipeline/meson.build
     create mode 100644 examples/pipeline/obj.c
     create mode 100644 examples/pipeline/obj.h
     create mode 100644 examples/pipeline/thread.c
     create mode 100644 examples/pipeline/thread.h
     create mode 100644 lib/librte_pipeline/rte_swx_ctl.c
     create mode 100644 lib/librte_pipeline/rte_swx_ctl.h
     create mode 100644 lib/librte_pipeline/rte_swx_extern.h
     create mode 100644 lib/librte_pipeline/rte_swx_pipeline.c
     create mode 100644 lib/librte_pipeline/rte_swx_pipeline.h
     create mode 100644 lib/librte_pipeline/rte_swx_pipeline_spec.c
     create mode 100644 lib/librte_port/rte_swx_port.h
     create mode 100644 lib/librte_port/rte_swx_port_ethdev.c
     create mode 100644 lib/librte_port/rte_swx_port_ethdev.h
     create mode 100644 lib/librte_port/rte_swx_port_source_sink.c
     create mode 100644 lib/librte_port/rte_swx_port_source_sink.h
     create mode 100644 lib/librte_table/rte_swx_table.h
     create mode 100644 lib/librte_table/rte_swx_table_em.c
     create mode 100644 lib/librte_table/rte_swx_table_em.h

    -- 
    2.17.1
Cristian Dumitrescu Sept. 22, 2020, 8:08 p.m. UTC | #2 
> -----Original Message-----
> From: Wang, Han2 <han2.wang@intel.com>
> Sent: Tuesday, September 22, 2020 9:06 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 00/41] Pipeline alignment with the P4
> language
> 
> Cristian
> 
> Thanks for updating the patch set. As I mentioned in my previous email, we
> are working on adding support for this as a new P4C CPU target. We plan to
> open source the compiler backend in the near future.
> 
> Cheers,
> Han
> 

Hi Han,

That is great news, thanks very much for your work!

Regards,
Cristian
David Marchand Sept. 23, 2020, 11:45 a.m. UTC | #3 
Hello Cristian,

On Thu, Sep 10, 2020 at 5:27 PM Cristian Dumitrescu
<cristian.dumitrescu@intel.com> wrote:
> Cristian Dumitrescu (41):
>   pipeline: add new SWX pipeline type
>   pipeline: add SWX pipeline input port
>   pipeline: add SWX pipeline output port
>   pipeline: add SWX headers and meta-data
>   pipeline: add SWX extern objects and funcs
>   pipeline: add SWX pipeline action
>   pipeline: add SWX pipeline tables
>   pipeline: add SWX pipeline instructions
>   pipeline: add SWX rx and extract instructions
>   pipeline: add SWX tx and emit instructions
>   pipeline: add header validate and invalidate SWX instructions
>   pipeline: add SWX mov instruction
>   pipeline: add SWX dma instruction
>   pipeline: introduce SWX add instruction
>   pipeline: introduce SWX sub instruction
>   pipeline: introduce SWX ckadd instruction
>   pipeline: introduce SWX cksub instruction
>   pipeline: introduce SWX and instruction
>   pipeline: introduce SWX or instruction
>   pipeline: introduce SWX xor instruction
>   pipeline: introduce SWX shl instruction
>   pipeline: introduce SWX shr instruction
>   pipeline: introduce SWX table instruction
>   pipeline: introduce SWX extern instruction
>   pipeline: introduce SWX jmp and return instructions
>   pipeline: add SWX instruction description
>   pipeline: add SWX instruction verifier
>   pipeline: add SWX instruction optimizer
>   pipeline: add SWX pipeline query API
>   pipeline: add SWX pipeline flush
>   pipeline: add SWX table update high level API
>   pipeline: add SWX pipeline specification file
>   port: add ethernet device SWX port
>   port: add source and sink SWX ports
>   table: add exact match SWX table
>   examples/pipeline: add new example application
>   examples/pipeline: add message passing mechanism
>   examples/pipeline: add configuration commands
>   examples/pipeline: add l2fwd example
>   examples/pipeline: add l2fwd with MAC swap example
>   examples/pipeline: add VXLAN encapsulation example

- This new feature is the future of the pipeline library: it will need
unit tests and/or tests in the CI to catch regressions.

- Many MACRO_WITH_FLOW_CONTROL warnings reported by checkpatches.

- On the patch titles, check-git-log.sh reports:
Wrong headline case:
            "pipeline: add SWX dma instruction": dma --> DMA
Wrong headline case:
            "pipeline: add SWX rx and extract instructions": rx --> Rx
Wrong headline case:
            "pipeline: add SWX tx and emit instructions": tx --> Tx
Wrong headline case:
            "pipeline: introduce SWX xor instruction": xor --> XOR

- We have yet another new example, please declare it in MAINTAINERS.

- I checked per patch compilation which is fine, thanks.
Cristian Dumitrescu Sept. 23, 2020, 4:07 p.m. UTC | #4 
Hi David,

Thanks for looking at this patch series!

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, September 23, 2020 12:46 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v4 00/41] Pipeline alignment with the P4
> language
> 
> Hello Cristian,
> 
> On Thu, Sep 10, 2020 at 5:27 PM Cristian Dumitrescu
> <cristian.dumitrescu@intel.com> wrote:
> > Cristian Dumitrescu (41):
> >   pipeline: add new SWX pipeline type
> >   pipeline: add SWX pipeline input port
> >   pipeline: add SWX pipeline output port
> >   pipeline: add SWX headers and meta-data
> >   pipeline: add SWX extern objects and funcs
> >   pipeline: add SWX pipeline action
> >   pipeline: add SWX pipeline tables
> >   pipeline: add SWX pipeline instructions
> >   pipeline: add SWX rx and extract instructions
> >   pipeline: add SWX tx and emit instructions
> >   pipeline: add header validate and invalidate SWX instructions
> >   pipeline: add SWX mov instruction
> >   pipeline: add SWX dma instruction
> >   pipeline: introduce SWX add instruction
> >   pipeline: introduce SWX sub instruction
> >   pipeline: introduce SWX ckadd instruction
> >   pipeline: introduce SWX cksub instruction
> >   pipeline: introduce SWX and instruction
> >   pipeline: introduce SWX or instruction
> >   pipeline: introduce SWX xor instruction
> >   pipeline: introduce SWX shl instruction
> >   pipeline: introduce SWX shr instruction
> >   pipeline: introduce SWX table instruction
> >   pipeline: introduce SWX extern instruction
> >   pipeline: introduce SWX jmp and return instructions
> >   pipeline: add SWX instruction description
> >   pipeline: add SWX instruction verifier
> >   pipeline: add SWX instruction optimizer
> >   pipeline: add SWX pipeline query API
> >   pipeline: add SWX pipeline flush
> >   pipeline: add SWX table update high level API
> >   pipeline: add SWX pipeline specification file
> >   port: add ethernet device SWX port
> >   port: add source and sink SWX ports
> >   table: add exact match SWX table
> >   examples/pipeline: add new example application
> >   examples/pipeline: add message passing mechanism
> >   examples/pipeline: add configuration commands
> >   examples/pipeline: add l2fwd example
> >   examples/pipeline: add l2fwd with MAC swap example
> >   examples/pipeline: add VXLAN encapsulation example
> 
> - This new feature is the future of the pipeline library: it will need
> unit tests and/or tests in the CI to catch regressions.
> 

Agreed, this is work in progress and will materialize in new test cases upstreamed into DPDK Test Framework (DTS)  during the 20.11 and 21.02 time frame. They are based on the new sample app introduced by this patch set, which already has some tests (see examples/pipeline/examples folder), but having them automated into DTS is WIP.

> - Many MACRO_WITH_FLOW_CONTROL warnings reported by checkpatches.
> 

Yes, I fixed all the other code style issues, this is the only one remaining. It is basically due to a recurring CHECK() macro. And it will also ripples over the code, so IMO it is time consuming & error prone to remove with no real benefit.

We also already have many places in DPDK that use the same pattern. I suggest we ignore this warning, are you OK with it?

> - On the patch titles, check-git-log.sh reports:
> Wrong headline case:
>             "pipeline: add SWX dma instruction": dma --> DMA
> Wrong headline case:
>             "pipeline: add SWX rx and extract instructions": rx --> Rx
> Wrong headline case:
>             "pipeline: add SWX tx and emit instructions": tx --> Tx
> Wrong headline case:
>             "pipeline: introduce SWX xor instruction": xor --> XOR
> 

I can do this change, but IMO it is not the right choice here, as in this particular case we have instructions that are called "rx", "tx", "dma", "and", "or", "xor", etc. So it is the name of an instruction rather than a text abbreviation. Hence, I think these messages are not really applicable here. What do you think?

> - We have yet another new example, please declare it in MAINTAINERS.
> 

Yes, will fix in V5.

> - I checked per patch compilation which is fine, thanks.
> 

Great, thanks!

> 
> --
> David Marchand

Regards,
Cristian
David Marchand Sept. 23, 2020, 4:28 p.m. UTC | #5 
On Wed, Sep 23, 2020 at 6:08 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
> > - Many MACRO_WITH_FLOW_CONTROL warnings reported by checkpatches.
> >
>
> Yes, I fixed all the other code style issues, this is the only one remaining. It is basically due to a recurring CHECK() macro. And it will also ripples over the code, so IMO it is time consuming & error prone to remove with no real benefit.
>
> We also already have many places in DPDK that use the same pattern. I suggest we ignore this warning, are you OK with it?

I am fine with ignoring, this is not like we have no other occurrence
of such macros.
I still see little value in those specific macros.


> > - On the patch titles, check-git-log.sh reports:
> > Wrong headline case:
> >             "pipeline: add SWX dma instruction": dma --> DMA
> > Wrong headline case:
> >             "pipeline: add SWX rx and extract instructions": rx --> Rx
> > Wrong headline case:
> >             "pipeline: add SWX tx and emit instructions": tx --> Tx
> > Wrong headline case:
> >             "pipeline: introduce SWX xor instruction": xor --> XOR
> >
>
> I can do this change, but IMO it is not the right choice here, as in this particular case we have instructions that are called "rx", "tx", "dma", "and", "or", "xor", etc. So it is the name of an instruction rather than a text abbreviation. Hence, I think these messages are not really applicable here. What do you think?

For this reason I am ok with ignoring too, Thomas wdyt?
Thomas Monjalon Sept. 23, 2020, 4:40 p.m. UTC | #6 
23/09/2020 18:28, David Marchand:
> On Wed, Sep 23, 2020 at 6:08 PM Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com> wrote:
> > > - On the patch titles, check-git-log.sh reports:
> > > Wrong headline case:
> > >             "pipeline: add SWX dma instruction": dma --> DMA
> > > Wrong headline case:
> > >             "pipeline: add SWX rx and extract instructions": rx --> Rx
> > > Wrong headline case:
> > >             "pipeline: add SWX tx and emit instructions": tx --> Tx
> > > Wrong headline case:
> > >             "pipeline: introduce SWX xor instruction": xor --> XOR
> > >
> >
> > I can do this change, but IMO it is not the right choice here, as in this particular case we have instructions that are called "rx", "tx", "dma", "and", "or", "xor", etc. So it is the name of an instruction rather than a text abbreviation. Hence, I think these messages are not really applicable here. What do you think?
> 
> For this reason I am ok with ignoring too, Thomas wdyt?

The general idea of titles is to not use exact same wording as in code
(no function or variable names for instance).
For the instructions, I don't know.
If you think it is better as lowercase, I can be convinced.
Cristian Dumitrescu Sept. 23, 2020, 4:49 p.m. UTC | #7 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, September 23, 2020 5:40 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; David Marchand
> <david.marchand@redhat.com>
> Cc: dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH v4 00/41] Pipeline alignment with the P4
> language
> 
> 23/09/2020 18:28, David Marchand:
> > On Wed, Sep 23, 2020 at 6:08 PM Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com> wrote:
> > > > - On the patch titles, check-git-log.sh reports:
> > > > Wrong headline case:
> > > >             "pipeline: add SWX dma instruction": dma --> DMA
> > > > Wrong headline case:
> > > >             "pipeline: add SWX rx and extract instructions": rx --> Rx
> > > > Wrong headline case:
> > > >             "pipeline: add SWX tx and emit instructions": tx --> Tx
> > > > Wrong headline case:
> > > >             "pipeline: introduce SWX xor instruction": xor --> XOR
> > > >
> > >
> > > I can do this change, but IMO it is not the right choice here, as in this
> particular case we have instructions that are called "rx", "tx", "dma", "and",
> "or", "xor", etc. So it is the name of an instruction rather than a text
> abbreviation. Hence, I think these messages are not really applicable here.
> What do you think?
> >
> > For this reason I am ok with ignoring too, Thomas wdyt?
> 
> The general idea of titles is to not use exact same wording as in code
> (no function or variable names for instance).
> For the instructions, I don't know.
> If you think it is better as lowercase, I can be convinced.
> 

OK, let me do the change in V5, thanks!
Cristian Dumitrescu Sept. 23, 2020, 7:02 p.m. UTC | #8 
> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Wednesday, September 23, 2020 5:49 PM
> To: 'Thomas Monjalon' <thomas@monjalon.net>; David Marchand
> <david.marchand@redhat.com>
> Cc: dev <dev@dpdk.org>
> Subject: RE: [dpdk-dev] [PATCH v4 00/41] Pipeline alignment with the P4
> language
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Wednesday, September 23, 2020 5:40 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; David Marchand
> > <david.marchand@redhat.com>
> > Cc: dev <dev@dpdk.org>
> > Subject: Re: [dpdk-dev] [PATCH v4 00/41] Pipeline alignment with the P4
> > language
> >
> > 23/09/2020 18:28, David Marchand:
> > > On Wed, Sep 23, 2020 at 6:08 PM Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com> wrote:
> > > > > - On the patch titles, check-git-log.sh reports:
> > > > > Wrong headline case:
> > > > >             "pipeline: add SWX dma instruction": dma --> DMA
> > > > > Wrong headline case:
> > > > >             "pipeline: add SWX rx and extract instructions": rx --> Rx
> > > > > Wrong headline case:
> > > > >             "pipeline: add SWX tx and emit instructions": tx --> Tx
> > > > > Wrong headline case:
> > > > >             "pipeline: introduce SWX xor instruction": xor --> XOR
> > > > >
> > > >
> > > > I can do this change, but IMO it is not the right choice here, as in this
> > particular case we have instructions that are called "rx", "tx", "dma", "and",
> > "or", "xor", etc. So it is the name of an instruction rather than a text
> > abbreviation. Hence, I think these messages are not really applicable here.
> > What do you think?
> > >
> > > For this reason I am ok with ignoring too, Thomas wdyt?
> >
> > The general idea of titles is to not use exact same wording as in code
> > (no function or variable names for instance).
> > For the instructions, I don't know.
> > If you think it is better as lowercase, I can be convinced.
> >
> 
> OK, let me do the change in V5, thanks!

Done in V5 just sent, thanks!