[v1,2/8] net/mlx5: add mlx5 Linux specific file with getter functions
Checks
Commit Message
'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
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?
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.
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.
@@ -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
new file mode 100644
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright 2020 Mellanox Technologies, Ltd
+
+includes += include_directories('.')
+sources += files(
+ 'mlx5_os.c',
+)
+
new file mode 100644
@@ -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;
+}
@@ -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)
@@ -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.
@@ -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_ */
@@ -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;