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/>

quoted

quoted

quoted

--- 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.

quoted

quoted

quoted

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

quoted

 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

quoted

 		$(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.

quoted

quoted

quoted

-
 # Create mlx5_autoconf.h or update it in case it differs from the new one.

 mlx5_autoconf.h: mlx5_autoconf.h.new

<snip/>

quoted

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

quoted

  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH

DAMAGE.

quoted

  */
-
+#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.

quoted

quoted

quoted

 /* 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