Re: [dpdk-dev] [PATCH v3 00/25] Refactor mlx5 to improve performance

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Jun 22, 2016 at 10:20:54AM +0200, Adrien Mazarguil wrote:
> On Tue, Jun 21, 2016 at 05:42:29PM +0100, Ferruh Yigit wrote:
> > On 6/21/2016 8:23 AM, Nelio Laranjeiro wrote:
> > > Enhance mlx5 with a data path that bypasses Verbs.
> > > 
> > > The first half of this patchset removes support for functionality completely
> > > rewritten in the second half (scatter/gather, inline send), while the data
> > > path is refactored without Verbs.
> > > 
> > > The PMD remains usable during the transition.
> > > 
> > > This patchset must be applied after "Miscellaneous fixes for mlx4 and mlx5".
> > > 
> > > Changes in v3:
> > > - Rebased patchset on top of next-net/rel_16_07.
> > > 
> > > Changes in v2:
> > > - Rebased patchset on top of dpdk/master.
> > > - Fixed CQE size on Power8.
> > > - Fixed mbuf assertion failure in debug mode.
> > > - Fixed missing class_id field in rte_pci_id by using RTE_PCI_DEVICE.
> > > 
> > > Adrien Mazarguil (8):
> > >   mlx5: replace countdown with threshold for Tx completions
> > >   mlx5: add debugging information about Tx queues capabilities
> > >   mlx5: check remaining space while processing Tx burst
> > >   mlx5: resurrect Tx gather support
> > >   mlx5: work around spurious compilation errors
> > >   mlx5: remove redundant Rx queue initialization code
> > >   mlx5: make Rx queue reinitialization safer
> > >   mlx5: resurrect Rx scatter support
> > > 
> > > Nelio Laranjeiro (16):
> > >   drivers: fix PCI class id support
> > >   mlx5: split memory registration function
> > >   mlx5: remove Tx gather support
> > >   mlx5: remove Rx scatter support
> > >   mlx5: remove configuration variable
> > >   mlx5: remove inline Tx support
> > >   mlx5: split Tx queue structure
> > >   mlx5: split Rx queue structure
> > >   mlx5: update prerequisites for upcoming enhancements
> > >   mlx5: add definitions for data path without Verbs
> > >   mlx5: add support for configuration through kvargs
> > >   mlx5: add Tx/Rx burst function selection wrapper
> > >   mlx5: refactor Rx data path
> > >   mlx5: refactor Tx data path
> > >   mlx5: handle Rx CQE compression
> > >   mlx5: add support for multi-packet send
> > > 
> > > Yaacov Hazan (1):
> > >   mlx5: add support for inline send
> > > 
> > 
> > Patchset applies and compiles fine, thanks.
> > 
> > But still has some checkpatch warnings, -btw, I am using the checkpatch
> > script from latest master branch of Linux repo.
> > 
> > Following is the sample type of warnings (not instances, there are more
> > than one instance per type):
> 
> While Nelio is preparing a v4 to address the kvargs issue, the remaining
> warnings can be safely ignored.
> 
> A few of them are in relocated but unmodified code as this patchset
> refactors the entire PMD, others are documented. We settled on positive
> errno values internally because mlx5 uses syscalls and switching the sign
> bit all over the place quickly became unmanageable. They are made negative
> when returning from public callbacks (except for kvargs by mistake).
> 
> In short, we did run checkpatch, fixed a million warnings and other errors
> and left those on purpose, nothing to worry about.
> 

Yes, they are nothing to worry about, but at the same time, I fail to see why
most of them should not be fixed. Even if you are moving code, unless it's a whole
file it's not going to show up as a move in the diff, so some small changes
during the move can be ok.
 
> > WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
> > #112: FILE: drivers/net/mlx5/mlx5_mr.c:65:
> > +       unsigned mem_idx)
> > 
This looks easily fixable.

> > WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a
> > separate line
> > #288: FILE: drivers/net/mlx5/mlx5_mr.c:241:
> > +        * pointer is valid. */
> > 
I also think this should be fixed.

> > WARNING:USE_NEGATIVE_ERRNO: return of an errno should typically be
> > negative (ie: return -EINVAL)
> > #524: FILE: drivers/net/mlx5/mlx5_txq.c:265:
> > +               return EINVAL;
> > 
> > WARNING:LONG_LINE: line over 80 characters
> > #108: FILE: drivers/net/mlx5/mlx5_ethdev.c:1250:
> > +                               txq_ctrl->txq.stats.idx =
> > primary_txq->stats.idx;
> > 
> > WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should
> > probably be static const char * const
> > #88: FILE: drivers/net/mlx5/mlx5.c:281:
> > +       static const char *params[] = {
> > 
> > ERROR:ASSIGN_IN_IF: do not use assignment in if condition
> > #218: FILE: drivers/net/mlx5/mlx5_rxtx.c:92:
> > +               if (!ret || !(ret = ((*buf)[i] == magic[i])))
> > 
> > CHECK:SPACING: spaces preferred around that '&' (ctx:VxV)
> > #414: FILE: drivers/net/mlx5/mlx5_rxtx.c:625:
> > +                               (uintptr_t)&(*rxq->cqes)[rxq->cq_ci &
> >                                            ^
> > 
> > WARNING:INDENTED_LABEL: labels should not be indented
> > #520: FILE: drivers/net/mlx5/mlx5_rxtx.c:789:
> > +       skip:
This I also feel should be fixed too.

Regards,
/Bruce