Re: [dpdk-dev] [PATCH v2 2/2] net/enic: add AVX2 based vectorized Rx handler

[Hyong Youb Kim <hyonkim@cisco.com>]

On Tue, Oct 02, 2018 at 05:08:21PM +0100, Ferruh Yigit wrote:
> On 9/28/2018 8:25 PM, John Daley wrote:
> > From: Hyong Youb Kim <hyonkim@cisco.com>
> > 
> > Add the vectorized version of the no-scatter Rx handler. It aims to
> > process 8 descriptors per loop using AVX2 SIMD instructions. This
> > handler is in its own file enic_rxtx_vec_avx2.c, and makefile and
> > meson.build are modified to compile it when the compiler supports
> > AVX2. Under ideal conditions, the vectorized handler reduces
> > cycles/packet by more than 30%, when compared against the no-scatter
> > Rx handler. Most implementation ideas come from i40e's AVX2 based
> > handler, so credit goes to its authors.
> > 
> > At this point, the new handler is meant for field trials, and is not
> > selected by default. So add a new devarg enable-avx2-rx to allow the
> > user to request the use of the new handler. When enable-avx2-rx=1, the
> > driver will consider using the new handler.
> > 
> > Also update the guide doc and introduce the vectorized handler.
> > 
> > Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> > Reviewed-by: John Daley <johndale@cisco.com>
> 
> <...>
>
[...]
> > +- Set ``devargs`` parameter ``enable-avx2-rx=1`` to explicitly request that
> > +  PMD consider the vectorized handler when selecting the receive handler.
> 
> Can be good to show a usage sample, as done in disable-overlay=1, like
> -w 12:00.0,enable-avx2-rx=1
> 

Fixed.

> > +
> > +  As the current implementation is intended for field trials, by default, the
> > +  vectorized handler is not considerd (``enable-avx2-rx=0``).
> > +
> > +- Run on a UCS M4 or later server with CPUs that support AVX2.
> > +
> > +PMD selects the vectorized handler when the handler is compiled into
> > +the driver, the user requests its use via ``enable-avx2-rx=1``, CPU
> > +supports AVX2, and scatter Rx is not used. To verify that the
> 
> Code doesn't check if user requested DEV_RX_OFFLOAD_SCATTER, if so perhaps
> shouldn't enable vector path.
> 

The code below actually checks if scatter is used and selects the avx2
handler only if scatter is not used.

bool
enic_use_vector_rx_handler(struct enic *enic)
{
[...]
        /* Do not support scatter Rx */
        if (!(enic->rq_count > 0 && enic->rq[0].data_queue_enable == 0))
                return false;

For enic, data_queue_enable == 0 implies that scatter Rx is not
used. It covers two cases. First, scatter Rx offload
(DEV_RX_OFFLOAD_SCATTER) is not requested. Second, scatter offload is
requested (flag is set), but the scatter feature on the NIC is not
needed, hence not enabled because maximum receive packet
(max_rx_pkt_len) fits in one mbuf.

> > --- a/drivers/net/enic/Makefile
> > +++ b/drivers/net/enic/Makefile
> > @@ -39,4 +39,11 @@ SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_intr.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_rq.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_rss.c
> >  
> > +ifeq ($(findstring RTE_MACHINE_CPUFLAG_AVX2,$(CFLAGS)),RTE_MACHINE_CPUFLAG_AVX2)
> > +# The current implementation assumes 64-bit pointers
> > +ifeq ($(CONFIG_RTE_ARCH_X86_64),y)
> > +	SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_rxtx_vec_avx2.c
> > +endif
> > +endif
> 
> This is not exactly enough.
> 
> CFLAGS has MACHINE_CPUFLAG based on the parameters pass the compiler. This flag
> is what compiler supports.
> 
> For example if DPDK build for "default" architecture AVX2 won't be supported and
> your vector path won't be build at all. Please check i40e Makefile.
> 

Ah, thanks for the explanation. Fixed.

> > +
> >  include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
> > index 775cd5d55..665f5668a 100644
> > --- a/drivers/net/enic/enic.h
> > +++ b/drivers/net/enic/enic.h
> > @@ -106,6 +106,7 @@ struct enic {
> >  	struct vnic_dev_bar bar0;
> >  	struct vnic_dev *vdev;
> >  
> > +	uint64_t mbuf_initializer;
> 
> A comment can be good.
> 

Added a comment.

> > +	/*
> > +	 * mbuf_initializer contains const-after-init fields of
> > +	 * receive mbufs (i.e. 64 bits of fields from rearm_data).
> > +	 * It is currently used by the vectorized handler.
> > +	 */
> > +	mb_def.nb_segs = 1;
> > +	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
> > +	mb_def.port = enic->port_id;
> > +	rte_mbuf_refcnt_set(&mb_def, 1);
> > +	rte_compiler_barrier();
> > +	p = (uintptr_t)&mb_def.rearm_data;
> > +	enic->mbuf_initializer = *(uint64_t *)p;
> 
> What do you think wrapping this block with "enic->enable_avx2_rx" check, mostly
> to show the usage intention?
> 

Makes sense. Moved the block inside if.

> > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)) {
> > +		PMD_INIT_LOG(DEBUG, " use the non-scatter avx2"
> > +			     " Rx handler");
> 
> No need to split the log line.

Fixed.

Thanks for the review. John will review the new patch and submit v4.

-Hyong