diff mbox series

[v1,2/8] net/mlx5: add mlx5 Linux specific file with getter functions

Message ID 20200603150602.4686-3-ophirmu@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Raslan Darawsheh
Headers
Series mlx5 PMD multi OS support |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Ophir Munk June 3, 2020, 3:05 p.m. UTC 
  'ctx' type (field in 'struct mlx5_ctx_shared') is changed from 'struct
ibv_context *' to 'void *'.  'ctx' members which are verbs dependent
(e.g. device_name) will be accessed through getter functions which are
added to a new file under Linux directory: linux/mlx5_os.c.

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/Makefile          |  1 +
 drivers/net/mlx5/linux/meson.build |  8 ++++
 drivers/net/mlx5/linux/mlx5_os.c   | 87 ++++++++++++++++++++++++++++++++++++++
 drivers/net/mlx5/meson.build       |  5 ++-
 drivers/net/mlx5/mlx5.c            | 18 ++++----
 drivers/net/mlx5/mlx5.h            |  6 ++-
 drivers/net/mlx5/mlx5_mp.c         |  2 +-
 7 files changed, 115 insertions(+), 12 deletions(-)
 create mode 100644 drivers/net/mlx5/linux/meson.build
 create mode 100644 drivers/net/mlx5/linux/mlx5_os.c

Comments

Ferruh Yigit June 8, 2020, 11:20 a.m. UTC | #1 
On 6/3/2020 4:05 PM, Ophir Munk wrote:
> 'ctx' type (field in 'struct mlx5_ctx_shared') is changed from 'struct
> ibv_context *' to 'void *'.  'ctx' members which are verbs dependent
> (e.g. device_name) will be accessed through getter functions which are
> added to a new file under Linux directory: linux/mlx5_os.c.
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>

<...>

> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2015 6WIND S.A.
> + * Copyright 2020 Mellanox Technologies, Ltd
> + */

Just to double check if '6WIND' is copy/paste error in this new file?

<...>

> @@ -677,13 +677,14 @@ mlx5_dev_shared_handler_install(struct mlx5_dev_ctx_shared *sh)
>  	int flags;
>  
>  	sh->intr_handle.fd = -1;
> -	flags = fcntl(sh->ctx->async_fd, F_GETFL);
> -	ret = fcntl(sh->ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
> +	flags = fcntl(((struct ibv_context *)sh->ctx)->async_fd, F_GETFL);
> +	ret = fcntl(((struct ibv_context *)sh->ctx)->async_fd,
> +		    F_SETFL, flags | O_NONBLOCK);

As far as I understand you are trying to remove to the dependency to ibverbs, at
least in root level, linux/x.c will have that dependency. (I assume this is for
Windows support)
The 'mlx5_os_get_ctx_device_path()' wrapper seems can work for it but what is
the point of above usage, that you explicitly cast "void *" to "(struct
ibv_context *)", so you still keep the ibv dependency?
Ophir Munk June 9, 2020, 8:40 a.m. UTC | #2 
Hi Ferruh,
Please see inline

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, June 8, 2020 2:20 PM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Matan Azrad
> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v1 2/8] net/mlx5: add mlx5 Linux specific file
> with getter functions
> 
> On 6/3/2020 4:05 PM, Ophir Munk wrote:
> > 'ctx' type (field in 'struct mlx5_ctx_shared') is changed from 'struct
> > ibv_context *' to 'void *'.  'ctx' members which are verbs dependent
> > (e.g. device_name) will be accessed through getter functions which are
> > added to a new file under Linux directory: linux/mlx5_os.c.
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > Acked-by: Matan Azrad <matan@mellanox.com>
> 
> <...>
> 
> > @@ -0,0 +1,87 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2015 6WIND S.A.
> > + * Copyright 2020 Mellanox Technologies, Ltd  */
> 
> Just to double check if '6WIND' is copy/paste error in this new file?
> 

Some functions were moved from file mlx5.c (with 6WIND copyright) to this file
and renamed. 
Should 6WIND copyright be kept or removed in this file (mlx5_os.c)?

> <...>
> 
> > @@ -677,13 +677,14 @@ mlx5_dev_shared_handler_install(struct
> mlx5_dev_ctx_shared *sh)
> >  	int flags;
> >
> >  	sh->intr_handle.fd = -1;
> > -	flags = fcntl(sh->ctx->async_fd, F_GETFL);
> > -	ret = fcntl(sh->ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
> > +	flags = fcntl(((struct ibv_context *)sh->ctx)->async_fd, F_GETFL);
> > +	ret = fcntl(((struct ibv_context *)sh->ctx)->async_fd,
> > +		    F_SETFL, flags | O_NONBLOCK);
> 
> As far as I understand you are trying to remove to the dependency to ibverbs,
> at least in root level, linux/x.c will have that dependency. (I assume this is for
> Windows support) The 'mlx5_os_get_ctx_device_path()' wrapper seems can
> work for it but what is the point of above usage, that you explicitly cast "void
> *" to "(struct ibv_context *)", so you still keep the ibv dependency?

The reason for keeping an explicit cast for async_fd (and not creating a new getter API)
is that this code snippet will be moved under linux in next commits where no getter function is needed.
I wanted to avoid adding a getter function here and then remove it in a follow up commit.
Ferruh Yigit June 9, 2020, 8:43 a.m. UTC | #3 
On 6/9/2020 9:40 AM, Ophir Munk wrote:
> Hi Ferruh,
> Please see inline
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, June 8, 2020 2:20 PM
>> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Matan Azrad
>> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>
>> Subject: Re: [dpdk-dev] [PATCH v1 2/8] net/mlx5: add mlx5 Linux specific file
>> with getter functions
>>
>> On 6/3/2020 4:05 PM, Ophir Munk wrote:
>>> 'ctx' type (field in 'struct mlx5_ctx_shared') is changed from 'struct
>>> ibv_context *' to 'void *'.  'ctx' members which are verbs dependent
>>> (e.g. device_name) will be accessed through getter functions which are
>>> added to a new file under Linux directory: linux/mlx5_os.c.
>>>
>>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
>>> Acked-by: Matan Azrad <matan@mellanox.com>
>>
>> <...>
>>
>>> @@ -0,0 +1,87 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright 2015 6WIND S.A.
>>> + * Copyright 2020 Mellanox Technologies, Ltd  */
>>
>> Just to double check if '6WIND' is copy/paste error in this new file?
>>
> 
> Some functions were moved from file mlx5.c (with 6WIND copyright) to this file
> and renamed. 
> Should 6WIND copyright be kept or removed in this file (mlx5_os.c)?

No. I just want to confirm this is not just copy/paste error, which happens on
new files, but done intentionally. As you did this intentionally, that is good.

> 
>> <...>
>>
>>> @@ -677,13 +677,14 @@ mlx5_dev_shared_handler_install(struct
>> mlx5_dev_ctx_shared *sh)
>>>  	int flags;
>>>
>>>  	sh->intr_handle.fd = -1;
>>> -	flags = fcntl(sh->ctx->async_fd, F_GETFL);
>>> -	ret = fcntl(sh->ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
>>> +	flags = fcntl(((struct ibv_context *)sh->ctx)->async_fd, F_GETFL);
>>> +	ret = fcntl(((struct ibv_context *)sh->ctx)->async_fd,
>>> +		    F_SETFL, flags | O_NONBLOCK);
>>
>> As far as I understand you are trying to remove to the dependency to ibverbs,
>> at least in root level, linux/x.c will have that dependency. (I assume this is for
>> Windows support) The 'mlx5_os_get_ctx_device_path()' wrapper seems can
>> work for it but what is the point of above usage, that you explicitly cast "void
>> *" to "(struct ibv_context *)", so you still keep the ibv dependency?
> 
> The reason for keeping an explicit cast for async_fd (and not creating a new getter API)
> is that this code snippet will be moved under linux in next commits where no getter function is needed.
> I wanted to avoid adding a getter function here and then remove it in a follow up commit. 
> 

Make sense if it is removed later, thanks for clarification.
diff mbox series

Patch

diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
index c160e6b..115b66c 100644
--- a/drivers/net/mlx5/Makefile
+++ b/drivers/net/mlx5/Makefile
@@ -32,6 +32,7 @@  SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_flow_verbs.c
 SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_mp.c
 SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_utils.c
 SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_socket.c
+SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += linux/mlx5_os.c
 
 # Basic CFLAGS.
 CFLAGS += -O3
diff --git a/drivers/net/mlx5/linux/meson.build b/drivers/net/mlx5/linux/meson.build
new file mode 100644
index 0000000..2ea0792
--- /dev/null
+++ b/drivers/net/mlx5/linux/meson.build
@@ -0,0 +1,8 @@ 
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright 2020 Mellanox Technologies, Ltd
+
+includes += include_directories('.')
+sources += files(
+	'mlx5_os.c',
+)
+
diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
new file mode 100644
index 0000000..9443239
--- /dev/null
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -0,0 +1,87 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2015 6WIND S.A.
+ * Copyright 2020 Mellanox Technologies, Ltd
+ */
+
+#include <stddef.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <net/if.h>
+#include <sys/mman.h>
+#include <linux/rtnetlink.h>
+#include <fcntl.h>
+
+/* Verbs header. */
+/* ISO C doesn't support unnamed structs/unions, disabling -pedantic. */
+#ifdef PEDANTIC
+#pragma GCC diagnostic ignored "-Wpedantic"
+#endif
+#include <infiniband/verbs.h>
+#ifdef PEDANTIC
+#pragma GCC diagnostic error "-Wpedantic"
+#endif
+
+#include <rte_malloc.h>
+#include <rte_ethdev_driver.h>
+#include <rte_ethdev_pci.h>
+#include <rte_pci.h>
+#include <rte_bus_pci.h>
+#include <rte_common.h>
+#include <rte_kvargs.h>
+#include <rte_rwlock.h>
+#include <rte_spinlock.h>
+#include <rte_string_fns.h>
+#include <rte_alarm.h>
+
+#include <mlx5_glue.h>
+#include <mlx5_devx_cmds.h>
+#include <mlx5_common.h>
+
+#include "mlx5_defs.h"
+#include "mlx5.h"
+#include "mlx5_utils.h"
+#include "mlx5_rxtx.h"
+#include "mlx5_autoconf.h"
+#include "mlx5_mr.h"
+#include "mlx5_flow.h"
+#include "rte_pmd_mlx5.h"
+
+/**
+ * Get ibv device name. Given an ibv_context pointer - return a
+ * pointer to the corresponding device name.
+ *
+ * @param[in] ctx
+ *   Pointer to ibv context.
+ *
+ * @return
+ *   Pointer to device name if ctx is valid, NULL otherwise.
+ */
+const char *
+mlx5_os_get_ctx_device_name(void *ctx)
+{
+	if (!ctx)
+		return NULL;
+	return ((struct ibv_context *)ctx)->device->name;
+}
+
+/**
+ * Get ibv device path name. Given an ibv_context pointer - return a
+ * pointer to the corresponding device path name.
+ *
+ * @param[in] ctx
+ *   Pointer to ibv context.
+ *
+ * @return
+ *   Pointer to device path name if ctx is valid, NULL otherwise.
+ */
+const char *
+mlx5_os_get_ctx_device_path(void *ctx)
+{
+	if (!ctx)
+		return NULL;
+
+	return ((struct ibv_context *)ctx)->device->ibdev_path;
+}
diff --git a/drivers/net/mlx5/meson.build b/drivers/net/mlx5/meson.build
index 928663a..e71b2c5 100644
--- a/drivers/net/mlx5/meson.build
+++ b/drivers/net/mlx5/meson.build
@@ -2,9 +2,9 @@ 
 # Copyright 2018 6WIND S.A.
 # Copyright 2018 Mellanox Technologies, Ltd
 
-if not is_linux
+if not (is_linux or is_windows)
 	build = false
-	reason = 'only supported on Linux'
+	reason = 'only supported on Linux and Windows'
 	subdir_done()
 endif
 
@@ -52,3 +52,4 @@  if get_option('buildtype').contains('debug')
 else
 	cflags += [ '-UPEDANTIC' ]
 endif
+subdir(exec_env)
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index f942f92..95a34d1 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -677,13 +677,14 @@  mlx5_dev_shared_handler_install(struct mlx5_dev_ctx_shared *sh)
 	int flags;
 
 	sh->intr_handle.fd = -1;
-	flags = fcntl(sh->ctx->async_fd, F_GETFL);
-	ret = fcntl(sh->ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
+	flags = fcntl(((struct ibv_context *)sh->ctx)->async_fd, F_GETFL);
+	ret = fcntl(((struct ibv_context *)sh->ctx)->async_fd,
+		    F_SETFL, flags | O_NONBLOCK);
 	if (ret) {
 		DRV_LOG(INFO, "failed to change file descriptor async event"
 			" queue");
 	} else {
-		sh->intr_handle.fd = sh->ctx->async_fd;
+		sh->intr_handle.fd = ((struct ibv_context *)sh->ctx)->async_fd;
 		sh->intr_handle.type = RTE_INTR_HANDLE_EXT;
 		if (rte_intr_callback_register(&sh->intr_handle,
 					mlx5_dev_interrupt_handler, sh)) {
@@ -831,10 +832,10 @@  mlx5_alloc_shared_ibctx(const struct mlx5_dev_spawn_data *spawn,
 	}
 	sh->refcnt = 1;
 	sh->max_port = spawn->max_port;
-	strncpy(sh->ibdev_name, sh->ctx->device->name,
-		sizeof(sh->ibdev_name));
-	strncpy(sh->ibdev_path, sh->ctx->device->ibdev_path,
-		sizeof(sh->ibdev_path));
+	strncpy(sh->ibdev_name, mlx5_os_get_ctx_device_name(sh->ctx),
+		sizeof(sh->ibdev_name) - 1);
+	strncpy(sh->ibdev_path, mlx5_os_get_ctx_device_path(sh->ctx),
+		sizeof(sh->ibdev_path) - 1);
 	/*
 	 * Setting port_id to max unallowed value means
 	 * there is no interrupt subhandler installed for
@@ -1515,7 +1516,8 @@  mlx5_dev_close(struct rte_eth_dev *dev)
 		return;
 	DRV_LOG(DEBUG, "port %u closing device \"%s\"",
 		dev->data->port_id,
-		((priv->sh->ctx != NULL) ? priv->sh->ctx->device->name : ""));
+		((priv->sh->ctx != NULL) ?
+		mlx5_os_get_ctx_device_name(priv->sh->ctx) : ""));
 	/*
 	 * If default mreg copy action is removed at the stop stage,
 	 * the search will return none and nothing will be done anymore.
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 4f2ca15..d020c10 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -493,7 +493,7 @@  struct mlx5_dev_ctx_shared {
 	uint32_t refcnt;
 	uint32_t devx:1; /* Opened with DV. */
 	uint32_t max_port; /* Maximal IB device port index. */
-	struct ibv_context *ctx; /* Verbs/DV context. */
+	void *ctx; /* Verbs/DV/DevX context. */
 	struct ibv_pd *pd; /* Protection Domain. */
 	uint32_t pdn; /* Protection Domain number. */
 	uint32_t tdn; /* Transport Domain number. */
@@ -853,4 +853,8 @@  struct mlx5_flow_meter *mlx5_flow_meter_attach
 					 struct rte_flow_error *error);
 void mlx5_flow_meter_detach(struct mlx5_flow_meter *fm);
 
+/* mlx5_os.c */
+const char *mlx5_os_get_ctx_device_name(void *ctx);
+const char *mlx5_os_get_ctx_device_path(void *ctx);
+
 #endif /* RTE_PMD_MLX5_H_ */
diff --git a/drivers/net/mlx5/mlx5_mp.c b/drivers/net/mlx5/mlx5_mp.c
index 7ad322d..a2b5c40 100644
--- a/drivers/net/mlx5/mlx5_mp.c
+++ b/drivers/net/mlx5/mlx5_mp.c
@@ -52,7 +52,7 @@  mlx5_mp_primary_handle(const struct rte_mp_msg *mp_msg, const void *peer)
 	case MLX5_MP_REQ_VERBS_CMD_FD:
 		mp_init_msg(&priv->mp_id, &mp_res, param->type);
 		mp_res.num_fds = 1;
-		mp_res.fds[0] = priv->sh->ctx->cmd_fd;
+		mp_res.fds[0] = ((struct ibv_context *)priv->sh->ctx)->cmd_fd;
 		res->result = 0;
 		ret = rte_mp_reply(&mp_res, peer);
 		break;