Hi David,

Thanks for looking at this patch series!

> -----Original Message-----
> From: David Marchand <david.march...@redhat.com>
> Sent: Wednesday, September 23, 2020 12:46 PM
> To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>
> Cc: dev <dev@dpdk.org>; Thomas Monjalon <tho...@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.dumitre...@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

Reply via email to