Re: [dpdk-dev] [PATCH v4] net/mlx5: load libmlx5 and libibverbs in run-time

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi Shachar,

On Wed, Jan 03, 2018 at 03:00:46PM +0000, Shachar Beiser wrote:
<snip/>
> > > --- a/config/common_base
> > > +++ b/config/common_base
> > > @@ -236,6 +236,7 @@ CONFIG_RTE_LIBRTE_MLX4_TX_MP_CACHE=8
> > >  # Compile burst-oriented Mellanox ConnectX-4 & ConnectX-5 (MLX5) PMD
> > > #  CONFIG_RTE_LIBRTE_MLX5_PMD=n
> > > +CONFIG_RTE_LIBRTE_MLX5_DLL=y
> > >  CONFIG_RTE_LIBRTE_MLX5_DEBUG=n
> > >  CONFIG_RTE_LIBRTE_MLX5_TX_MP_CACHE=8
> > 
> > Not sure a new configuration item is allowed.  If it is, the documentation of
> > such variable is missing.
> 
> [S.B] The patch is based on this CONFIG_RTE_LIBRTE_MLX5_DLL , it was
> required by Adrian in the design phase to enable/disable this linkage
> mode.
>           I will update the documentation .

Before updating the documentation you should speak with Thomas or
Ferruh, not sure new items are allowed anymore.

> > > diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> > > index a3984eb..24fa127 100644
> > > --- a/drivers/net/mlx5/Makefile
> > > +++ b/drivers/net/mlx5/Makefile
> > > @@ -53,18 +53,25 @@ SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) +=
> > mlx5_rss.c
> > >  SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_mr.c
> > >  SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_flow.c
> > >  SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_socket.c
> > > -
> > > +ifeq ($(CONFIG_RTE_LIBRTE_MLX5_DLL),y)
> > > +SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += lib/mlx5_dll.c endif
> > >  # Basic CFLAGS.
> > >  CFLAGS += -O3
> > >  CFLAGS += -std=c11 -Wall -Wextra
> > >  CFLAGS += -g
> > >  CFLAGS += -I.
> > > +CFLAGS += -I$(SRCDIR)
> > >  CFLAGS += -D_BSD_SOURCE
> > >  CFLAGS += -D_DEFAULT_SOURCE
> > >  CFLAGS += -D_XOPEN_SOURCE=600
> > >  CFLAGS += $(WERROR_FLAGS)
> > >  CFLAGS += -Wno-strict-prototypes
> > > +ifeq ($(CONFIG_RTE_LIBRTE_MLX5_DLL),y) LDLIBS += -ldl else
> > >  LDLIBS += -libverbs -lmlx5
> > > +endif
> > >  LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring  LDLIBS +=
> > > -lrte_ethdev -lrte_net -lrte_kvargs  LDLIBS += -lrte_bus_pci @@
> > > -105,26 +112,28 @@ endif
> > >
> > >  mlx5_autoconf.h.new: FORCE
> > >
> > > +VERBS_H := infiniband/verbs.h
> > > +MLX5DV_H := infiniband/mlx5dv.h
> > >  mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
> > >  	$Q $(RM) -f -- '$@'
> > >  	$Q sh -- '$<' '$@' \
> > >  		HAVE_IBV_DEVICE_VXLAN_SUPPORT \
> > > -		infiniband/verbs.h \
> > > +		$(VERBS_H) \
> > >  		enum IBV_DEVICE_VXLAN_SUPPORT \
> > >  		$(AUTOCONF_OUTPUT)
> > >  	$Q sh -- '$<' '$@' \
> > >  		HAVE_IBV_WQ_FLAG_RX_END_PADDING \
> > > -		infiniband/verbs.h \
> > > +		$(VERBS_H) \
> > >  		enum IBV_WQ_FLAG_RX_END_PADDING \
> > >  		$(AUTOCONF_OUTPUT)
> > >  	$Q sh -- '$<' '$@' \
> > >  		HAVE_IBV_MLX5_MOD_MPW \
> > > -		infiniband/mlx5dv.h \
> > > +		$(MLX5DV_H) \
> > >  		enum MLX5DV_CONTEXT_FLAGS_MPW_ALLOWED \
> > >  		$(AUTOCONF_OUTPUT)
> > >  	$Q sh -- '$<' '$@' \
> > >  		HAVE_IBV_MLX5_MOD_CQE_128B_COMP \
> > > -		infiniband/mlx5dv.h \
> > > +		$(MLX5DV_H) \
> > >  		enum MLX5DV_CONTEXT_FLAGS_CQE_128B_COMP \
> > >  		$(AUTOCONF_OUTPUT)
> > >  	$Q sh -- '$<' '$@' \
> > > @@ -144,10 +153,9 @@ mlx5_autoconf.h.new:
> > $(RTE_SDK)/buildtools/auto-config-h.sh
> > >  		$(AUTOCONF_OUTPUT)
> > >  	$Q sh -- '$<' '$@' \
> > >  		HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> > > -		infiniband/verbs.h \
> > > +		$(VERBS_H)  \
> > >  		enum IBV_FLOW_SPEC_ACTION_COUNT \
> > >  		$(AUTOCONF_OUTPUT)
> > 
> > This modification should be inside its own patch, it is not directly related to
> > the this patch itself.
> [S.B] I can revert the VERBS_H ,  MLX5DV_H  and not change it all . 
>            There is no need to change in a different patch. 

As you wish.

> > > -
> > >  # Create mlx5_autoconf.h or update it in case it differs from the new one.
> > >
> > >  mlx5_autoconf.h: mlx5_autoconf.h.new
> > <snip/>
> > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > > cd66fe1..eeef782 100644
> > > --- a/drivers/net/mlx5/mlx5.c
> > > +++ b/drivers/net/mlx5/mlx5.c
> > > @@ -30,7 +30,8 @@
> > >   *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> > OF THE USE
> > >   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> > DAMAGE.
> > >   */
> > > -
> > > +#define _GNU_SOURCE
> > > +#include <stdio.h>
> > >  #include <stddef.h>
> > >  #include <unistd.h>
> > >  #include <string.h>
> > > @@ -39,13 +40,17 @@
> > >  #include <stdlib.h>
> > >  #include <errno.h>
> > >  #include <net/if.h>
> > > -
> > > +#include <dlfcn.h>
> > 
> > The empty line should remain.
> [S.B] OK.

To be sure, the empty line should be between the system include and
verbs ones.

> > 
> > >  /* Verbs header. */
> > >  /* ISO C doesn't support unnamed structs/unions, disabling -pedantic.
> > > */  #ifdef PEDANTIC  #pragma GCC diagnostic ignored "-Wpedantic"
> > >  #endif
> > > +#ifdef RTE_LIBRTE_MLX5_DLL
> > > +#include "lib/mlx5_dll.h"
> > > +#else
> > >  #include <infiniband/verbs.h>
> > > +#endif
> > 
> > This could be done by the mlx5_dll.h file which could include the correct
> > header according to the configuration.
> [S.B] I guess you refer to all *.c files that includes the verbs.h .
>            I will change them all ? 

Yes

<snip/>
 
Thanks,

-- 
Nélio Laranjeiro
6WIND