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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

Hello,

On Thu, Jan 04, 2018 at 08:36:08AM +0100, Nélio Laranjeiro wrote:
> 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

Actually, why this change? It shouldn't be necessary. As the patch is
defining functions with the very same prototype, it shouldn't matter
if their declarations are coming from one header or another.

  Marcelo