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

[Adrien Mazarguil]

On Wed, Jun 22, 2016 at 11:30:05AM +0200, Adrien Mazarguil wrote:
> On Wed, Jun 22, 2016 at 10:19:14AM +0100, Bruce Richardson wrote:
> > 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.
> 
> Right but it won't prevent some of them to still show up anyway. We will
> take them into account in the future.
> 
> Now, considering these warnings are minor and were already there before, do
> you think Nelio needs to spam the world with a v5?
> 
> Looks like he forgot "v4" in the subject of his last cover letter as well.

After discussing this with Nelio (and cooling down a bit), we decided to
submit a v5 to address the above warnings, except false positives.

Then for the remaining stuff, namely the fact we use positive errno values
internally and existing code that does not conform DPDK coding rules, we
will submit another set of patches for 16.11 to address them all at once.

-- 
Adrien Mazarguil
6WIND