Re: [dpdk-dev] [PATCH v4] net/mlx4: enhance Rx packet type offloads

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Sun, Nov 05, 2017 at 07:26:56PM +0200, Moti Haimovsky wrote:
> This patch enhances the Rx packet type offload to also report the L4
> protocol information in the hw ptype filled by the PMD for each received
> packet.
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>

Patch looks fine therefore:

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Although in the end you didn't initialize mlx4_ptype_table[] using a loop
(went all the way back to static array initialization) and I'm still
wondering why since you never replied on that topic.

Another comment: please remember to send patch updates as replies to their
previous iteration. If you search for "enhance Rx packet type offloads"
in [1], you notice they're not part of the same thread. While minor this
doesn't help the review process.

Regarding your previous reply about mlx4_dev_supported_ptypes_get():

[...]
> > > +
> > > +   if (dev->rx_pkt_burst == mlx4_rx_burst)
> > > +           return ptypes;
> > > +   return NULL;
> >
> > How about just returning the array regardless?
>
> From DPDK documentation (and as done in other drivers):
> http://dpdk.org/doc/api/rte__ethdev_8h.html#aa63202d322632467f9cc5fc460e04ea4
> Note
> Better to invoke this API after the device is already started or rx burst
> function is decided, to obtain correct supported ptypes.
> if a given PMD does not report what ptypes it supports, then the supported
> ptype count is reported as 0.
> 
> In our case rx_pkt_burst can also point to mlx4_rx_burst_removed

Right, and it doesn't hurt. My point was that mlx4_rx_burst_removed() is set
as a safety when closing/stopping the device, no more Rx in that case. The
mlx4 PMD currently exposes a single Rx function which does support
everything, not multiple optimized ones with fewer capabilities, which is
the actual reason behind checking the current Rx function.

[1] http://dpdk.org/ml/archives/dev/2017-November/

-- 
Adrien Mazarguil
6WIND