From: Yongseok Koh <yskoh@mellanox.com>
To: Shahaf Shuler <shahafs@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/4] net/mlx5: replace IPC socket with EAL API
Date: Mon, 18 Mar 2019 21:29:53 +0000 [thread overview]
Message-ID: <20190318212943.GB37866@yongseok-MBP.local> (raw)
In-Reply-To: <AM0PR0502MB379507347F8EFEC448B9AE10C34B0@AM0PR0502MB3795.eurprd05.prod.outlook.com>
On Thu, Mar 14, 2019 at 05:36:32AM -0700, Shahaf Shuler wrote:
> Hi Koh,
>
> Thursday, March 7, 2019 9:33 AM, Yongseok Koh:
> > Subject: [PATCH 2/4] net/mlx5: replace IPC socket with EAL API
> >
> > Socket API is used for IPC in order for secondary process to acquire Verb
> > command file descriptor. The FD is used to remap UAR address. The new
> > multi-process APIs (rte_mp) in EAL are newly introduced. mlx5_socket.c is
> > replaced with mlx5_mp.c, which uses the new APIs.
> >
> > As it is PMD global infrastructure, only one IPC channel is established.
> > All the IPC message types may have port_id in the message if there is need
> > to reference a specific device.
> >
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>
> [...]
>
> > /**
> > diff --git a/drivers/net/mlx5/mlx5_mp.c b/drivers/net/mlx5/mlx5_mp.c new
> > file mode 100644 index 0000000000..19a1f25f0e
> > --- /dev/null
> > +++ b/drivers/net/mlx5/mlx5_mp.c
> > @@ -0,0 +1,126 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2019 6WIND S.A.
> > + * Copyright 2019 Mellanox Technologies, Ltd */
> > +
> > +#include <assert.h>
> > +#include <stdio.h>
> > +#include <time.h>
> > +
> > +#include <rte_eal.h>
> > +#include <rte_ethdev_driver.h>
> > +#include <rte_string_fns.h>
> > +
> > +#include "mlx5.h"
> > +#include "mlx5_utils.h"
> > +
> > +/**
> > + * Initialize IPC message.
> > + *
> > + * @param[in] dev
> > + * Pointer to Ethernet structure.
> > + * @param[out] msg
> > + * Pointer to message to fill in.
> > + * @param[in] type
> > + * Message type.
> > + */
> > +static inline void
> > +mp_init_msg(struct rte_eth_dev *dev, struct rte_mp_msg *msg,
> > + enum mlx5_mp_req_type type)
> > +{
> > + struct mlx5_mp_param *param = (struct mlx5_mp_param *)msg-
> > >param;
> > +
> > + memset(msg, 0, sizeof(*msg));
> > + strlcpy(msg->name, MLX5_MP_NAME, sizeof(msg->name));
> > + msg->len_param = sizeof(*param);
> > + param->type = type;
> > + param->port_id = dev->data->port_id;
> > +}
> > +
> > +/**
> > + * IPC message handler of primary process.
> > + *
> > + * @param[in] dev
> > + * Pointer to Ethernet structure.
> > + * @param[in] peer
> > + * Pointer to the peer socket path.
> > + *
> > + * @return
> > + * 0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +static int
> > +mp_primary_handle(const struct rte_mp_msg *mp_msg, const void *peer)
> > {
> > + struct rte_mp_msg mp_res;
> > + struct mlx5_mp_param *res = (struct mlx5_mp_param
> > *)mp_res.param;
> > + const struct mlx5_mp_param *param =
> > + (const struct mlx5_mp_param *)mp_msg->param;
> > + struct rte_eth_dev *dev = &rte_eth_devices[param->port_id];
>
> Need to check dev is a valid one before we continue. If such case happen need error log (like you have for invalid req).
Good point.
> > + struct mlx5_priv *priv = dev->data->dev_private;
> > + int ret = 0;
> > +
> > + assert(rte_eal_process_type() == RTE_PROC_PRIMARY);
> > + switch (param->type) {
> > + case MLX5_MP_REQ_VERBS_CMD_FD:
> > + mp_init_msg(dev, &mp_res, param->type);
> > + mp_res.num_fds = 1;
> > + mp_res.fds[0] = priv->ctx->cmd_fd;
> > + res->result = 0;
> > + ret = rte_mp_reply(&mp_res, peer);
> > + break;
> > + default:
> > + rte_errno = EINVAL;
> > + DRV_LOG(ERR, "port %u invalid mp request type",
> > + dev->data->port_id);
> > + return -rte_errno;
> > + }
> > + return ret;
> > +}
> > +
> > +/**
> > + * Request Verbs command file descriptor for mmap to the primary process.
> > + *
> > + * @param[in] dev
> > + * Pointer to Ethernet structure.
> > + *
> > + * @return
> > + * fd on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +int
> > +mlx5_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev) {
> > + struct rte_mp_msg mp_req;
> > + struct rte_mp_msg *mp_res;
> > + struct rte_mp_reply mp_rep;
> > + struct mlx5_mp_param *res __rte_unused;
> > + struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
> > + int cmd_fd;
> > + int ret;
> > +
> > + assert(rte_eal_process_type() == RTE_PROC_SECONDARY);
> > + mp_init_msg(dev, &mp_req, MLX5_MP_REQ_VERBS_CMD_FD);
> > + ret = rte_mp_request_sync(&mp_req, &mp_rep, &ts);
> > + if (ret) {
> > + DRV_LOG(ERR,
> > + "port %u failed to get command FD from primary
> > process",
> > + dev->data->port_id);
> > + return -rte_errno;
> > + }
> > + assert(mp_rep.nb_received == 1);
> > + mp_res = &mp_rep.msgs[0];
> > + res = (struct mlx5_mp_param *)mp_res->param;
> > + assert(!res->result);
>
> Above should not be an assert rather and actual check.
The reason was because only zero is set by the handler. If the request is
successful, res->result has to have zero. Otherwise, that should be memory
corruption or bug in the rte_mp lib. So, I think it is better to have 'if'
instead of 'assert' so that we can prevent returning invalid cmd_fd.
Thanks,
Yongseok
> > + assert(mp_res->num_fds == 1);
> > + cmd_fd = mp_res->fds[0];
> > + free(mp_rep.msgs);
> > + DRV_LOG(DEBUG, "port %u command FD from primary is %d",
> > + dev->data->port_id, cmd_fd);
> > + return cmd_fd;
> > +}
> > +
> > +void
> > +mlx5_mp_init(void)
> > +{
> > + if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > + rte_mp_action_register(MLX5_MP_NAME,
> > mp_primary_handle); }
> > diff --git a/drivers/net/mlx5/mlx5_socket.c
> > b/drivers/net/mlx5/mlx5_socket.c deleted file mode 100644 index
> > 41cac3c6aa..0000000000
> > --- a/drivers/net/mlx5/mlx5_socket.c
> > +++ /dev/null
> > @@ -1,306 +0,0 @@
> > -/* SPDX-License-Identifier: BSD-3-Clause
> > - * Copyright 2016 6WIND S.A.
> > - * Copyright 2016 Mellanox Technologies, Ltd
> > - */
> > -
> > -#include <sys/types.h>
> > -#include <sys/socket.h>
> > -#include <sys/un.h>
> > -#include <fcntl.h>
> > -#include <stdio.h>
> > -#include <unistd.h>
> > -#include <sys/stat.h>
> > -
> > -#include "mlx5.h"
> > -#include "mlx5_utils.h"
> > -
> > -/**
> > - * Initialise the socket to communicate with the secondary process
> > - *
> > - * @param[in] dev
> > - * Pointer to Ethernet device.
> > - *
> > - * @return
> > - * 0 on success, a negative errno value otherwise and rte_errno is set.
> > - */
> > -int
> > -mlx5_socket_init(struct rte_eth_dev *dev) -{
> > - struct mlx5_priv *priv = dev->data->dev_private;
> > - struct sockaddr_un sun = {
> > - .sun_family = AF_UNIX,
> > - };
> > - int ret;
> > - int flags;
> > -
> > - /*
> > - * Close the last socket that was used to communicate
> > - * with the secondary process
> > - */
> > - if (priv->primary_socket)
> > - mlx5_socket_uninit(dev);
> > - /*
> > - * Initialise the socket to communicate with the secondary
> > - * process.
> > - */
> > - ret = socket(AF_UNIX, SOCK_STREAM, 0);
> > - if (ret < 0) {
> > - rte_errno = errno;
> > - DRV_LOG(WARNING, "port %u secondary process not
> > supported: %s",
> > - dev->data->port_id, strerror(errno));
> > - goto error;
> > - }
> > - priv->primary_socket = ret;
> > - flags = fcntl(priv->primary_socket, F_GETFL, 0);
> > - if (flags == -1) {
> > - rte_errno = errno;
> > - goto error;
> > - }
> > - ret = fcntl(priv->primary_socket, F_SETFL, flags | O_NONBLOCK);
> > - if (ret < 0) {
> > - rte_errno = errno;
> > - goto error;
> > - }
> > - snprintf(sun.sun_path, sizeof(sun.sun_path), "/var/tmp/%s_%d",
> > - MLX5_DRIVER_NAME, priv->primary_socket);
> > - remove(sun.sun_path);
> > - ret = bind(priv->primary_socket, (const struct sockaddr *)&sun,
> > - sizeof(sun));
> > - if (ret < 0) {
> > - rte_errno = errno;
> > - DRV_LOG(WARNING,
> > - "port %u cannot bind socket, secondary process not"
> > - " supported: %s",
> > - dev->data->port_id, strerror(errno));
> > - goto close;
> > - }
> > - ret = listen(priv->primary_socket, 0);
> > - if (ret < 0) {
> > - rte_errno = errno;
> > - DRV_LOG(WARNING, "port %u secondary process not
> > supported: %s",
> > - dev->data->port_id, strerror(errno));
> > - goto close;
> > - }
> > - return 0;
> > -close:
> > - remove(sun.sun_path);
> > -error:
> > - claim_zero(close(priv->primary_socket));
> > - priv->primary_socket = 0;
> > - return -rte_errno;
> > -}
> > -
> > -/**
> > - * Un-Initialise the socket to communicate with the secondary process
> > - *
> > - * @param[in] dev
> > - */
> > -void
> > -mlx5_socket_uninit(struct rte_eth_dev *dev) -{
> > - struct mlx5_priv *priv = dev->data->dev_private;
> > -
> > - MKSTR(path, "/var/tmp/%s_%d", MLX5_DRIVER_NAME, priv-
> > >primary_socket);
> > - claim_zero(close(priv->primary_socket));
> > - priv->primary_socket = 0;
> > - claim_zero(remove(path));
> > -}
> > -
> > -/**
> > - * Handle socket interrupts.
> > - *
> > - * @param dev
> > - * Pointer to Ethernet device.
> > - */
> > -void
> > -mlx5_socket_handle(struct rte_eth_dev *dev) -{
> > - struct mlx5_priv *priv = dev->data->dev_private;
> > - int conn_sock;
> > - int ret = 0;
> > - struct cmsghdr *cmsg = NULL;
> > - struct ucred *cred = NULL;
> > - char buf[CMSG_SPACE(sizeof(struct ucred))] = { 0 };
> > - char vbuf[1024] = { 0 };
> > - struct iovec io = {
> > - .iov_base = vbuf,
> > - .iov_len = sizeof(*vbuf),
> > - };
> > - struct msghdr msg = {
> > - .msg_iov = &io,
> > - .msg_iovlen = 1,
> > - .msg_control = buf,
> > - .msg_controllen = sizeof(buf),
> > - };
> > - int *fd;
> > -
> > - /* Accept the connection from the client. */
> > - conn_sock = accept(priv->primary_socket, NULL, NULL);
> > - if (conn_sock < 0) {
> > - DRV_LOG(WARNING, "port %u connection failed: %s",
> > - dev->data->port_id, strerror(errno));
> > - return;
> > - }
> > - ret = setsockopt(conn_sock, SOL_SOCKET, SO_PASSCRED, &(int){1},
> > - sizeof(int));
> > - if (ret < 0) {
> > - ret = errno;
> > - DRV_LOG(WARNING, "port %u cannot change socket
> > options: %s",
> > - dev->data->port_id, strerror(rte_errno));
> > - goto error;
> > - }
> > - ret = recvmsg(conn_sock, &msg, MSG_WAITALL);
> > - if (ret < 0) {
> > - ret = errno;
> > - DRV_LOG(WARNING, "port %u received an empty message:
> > %s",
> > - dev->data->port_id, strerror(rte_errno));
> > - goto error;
> > - }
> > - /* Expect to receive credentials only. */
> > - cmsg = CMSG_FIRSTHDR(&msg);
> > - if (cmsg == NULL) {
> > - DRV_LOG(WARNING, "port %u no message", dev->data-
> > >port_id);
> > - goto error;
> > - }
> > - if ((cmsg->cmsg_type == SCM_CREDENTIALS) &&
> > - (cmsg->cmsg_len >= sizeof(*cred))) {
> > - cred = (struct ucred *)CMSG_DATA(cmsg);
> > - assert(cred != NULL);
> > - }
> > - cmsg = CMSG_NXTHDR(&msg, cmsg);
> > - if (cmsg != NULL) {
> > - DRV_LOG(WARNING, "port %u message wrongly
> > formatted",
> > - dev->data->port_id);
> > - goto error;
> > - }
> > - /* Make sure all the ancillary data was received and valid. */
> > - if ((cred == NULL) || (cred->uid != getuid()) ||
> > - (cred->gid != getgid())) {
> > - DRV_LOG(WARNING, "port %u wrong credentials",
> > - dev->data->port_id);
> > - goto error;
> > - }
> > - /* Set-up the ancillary data. */
> > - cmsg = CMSG_FIRSTHDR(&msg);
> > - assert(cmsg != NULL);
> > - cmsg->cmsg_level = SOL_SOCKET;
> > - cmsg->cmsg_type = SCM_RIGHTS;
> > - cmsg->cmsg_len = CMSG_LEN(sizeof(priv->ctx->cmd_fd));
> > - fd = (int *)CMSG_DATA(cmsg);
> > - *fd = priv->ctx->cmd_fd;
> > - ret = sendmsg(conn_sock, &msg, 0);
> > - if (ret < 0)
> > - DRV_LOG(WARNING, "port %u cannot send response",
> > - dev->data->port_id);
> > -error:
> > - close(conn_sock);
> > -}
> > -
> > -/**
> > - * Connect to the primary process.
> > - *
> > - * @param[in] dev
> > - * Pointer to Ethernet structure.
> > - *
> > - * @return
> > - * fd on success, negative errno value otherwise and rte_errno is set.
> > - */
> > -int
> > -mlx5_socket_connect(struct rte_eth_dev *dev) -{
> > - struct mlx5_priv *priv = dev->data->dev_private;
> > - struct sockaddr_un sun = {
> > - .sun_family = AF_UNIX,
> > - };
> > - int socket_fd = -1;
> > - int *fd = NULL;
> > - int ret;
> > - struct ucred *cred;
> > - char buf[CMSG_SPACE(sizeof(*cred))] = { 0 };
> > - char vbuf[1024] = { 0 };
> > - struct iovec io = {
> > - .iov_base = vbuf,
> > - .iov_len = sizeof(*vbuf),
> > - };
> > - struct msghdr msg = {
> > - .msg_control = buf,
> > - .msg_controllen = sizeof(buf),
> > - .msg_iov = &io,
> > - .msg_iovlen = 1,
> > - };
> > - struct cmsghdr *cmsg;
> > -
> > - ret = socket(AF_UNIX, SOCK_STREAM, 0);
> > - if (ret < 0) {
> > - rte_errno = errno;
> > - DRV_LOG(WARNING, "port %u cannot connect to primary",
> > - dev->data->port_id);
> > - goto error;
> > - }
> > - socket_fd = ret;
> > - snprintf(sun.sun_path, sizeof(sun.sun_path), "/var/tmp/%s_%d",
> > - MLX5_DRIVER_NAME, priv->primary_socket);
> > - ret = connect(socket_fd, (const struct sockaddr *)&sun, sizeof(sun));
> > - if (ret < 0) {
> > - rte_errno = errno;
> > - DRV_LOG(WARNING, "port %u cannot connect to primary",
> > - dev->data->port_id);
> > - goto error;
> > - }
> > - cmsg = CMSG_FIRSTHDR(&msg);
> > - if (cmsg == NULL) {
> > - rte_errno = EINVAL;
> > - DRV_LOG(DEBUG, "port %u cannot get first message",
> > - dev->data->port_id);
> > - goto error;
> > - }
> > - cmsg->cmsg_level = SOL_SOCKET;
> > - cmsg->cmsg_type = SCM_CREDENTIALS;
> > - cmsg->cmsg_len = CMSG_LEN(sizeof(*cred));
> > - cred = (struct ucred *)CMSG_DATA(cmsg);
> > - if (cred == NULL) {
> > - rte_errno = EINVAL;
> > - DRV_LOG(DEBUG, "port %u no credentials received",
> > - dev->data->port_id);
> > - goto error;
> > - }
> > - cred->pid = getpid();
> > - cred->uid = getuid();
> > - cred->gid = getgid();
> > - ret = sendmsg(socket_fd, &msg, MSG_DONTWAIT);
> > - if (ret < 0) {
> > - rte_errno = errno;
> > - DRV_LOG(WARNING,
> > - "port %u cannot send credentials to primary: %s",
> > - dev->data->port_id, strerror(errno));
> > - goto error;
> > - }
> > - ret = recvmsg(socket_fd, &msg, MSG_WAITALL);
> > - if (ret <= 0) {
> > - rte_errno = errno;
> > - DRV_LOG(WARNING, "port %u no message from primary:
> > %s",
> > - dev->data->port_id, strerror(errno));
> > - goto error;
> > - }
> > - cmsg = CMSG_FIRSTHDR(&msg);
> > - if (cmsg == NULL) {
> > - rte_errno = EINVAL;
> > - DRV_LOG(WARNING, "port %u no file descriptor received",
> > - dev->data->port_id);
> > - goto error;
> > - }
> > - fd = (int *)CMSG_DATA(cmsg);
> > - if (*fd < 0) {
> > - DRV_LOG(WARNING, "port %u no file descriptor received:
> > %s",
> > - dev->data->port_id, strerror(errno));
> > - rte_errno = *fd;
> > - goto error;
> > - }
> > - ret = *fd;
> > - close(socket_fd);
> > - return ret;
> > -error:
> > - if (socket_fd != -1)
> > - close(socket_fd);
> > - return -rte_errno;
> > -}
> > --
> > 2.11.0
>
next prev parent reply other threads:[~2019-03-18 21:29 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-07 7:33 [dpdk-dev] [PATCH 0/4] net/mlx5: rework IPC socket and PMD global data init Yongseok Koh
2019-03-07 7:33 ` [dpdk-dev] [PATCH 1/4] net/mlx5: fix memory event on secondary process Yongseok Koh
2019-03-07 7:33 ` [dpdk-dev] [PATCH 2/4] net/mlx5: replace IPC socket with EAL API Yongseok Koh
2019-03-14 12:36 ` Shahaf Shuler
2019-03-14 12:36 ` Shahaf Shuler
2019-03-18 21:29 ` Yongseok Koh [this message]
2019-03-18 21:29 ` Yongseok Koh
2019-03-07 7:33 ` [dpdk-dev] [PATCH 3/4] net/mlx5: rework PMD global data init Yongseok Koh
2019-03-14 12:36 ` Shahaf Shuler
2019-03-14 12:36 ` Shahaf Shuler
2019-03-18 21:21 ` Yongseok Koh
2019-03-18 21:21 ` Yongseok Koh
2019-03-19 6:54 ` Shahaf Shuler
2019-03-19 6:54 ` Shahaf Shuler
2019-03-07 7:33 ` [dpdk-dev] [PATCH 4/4] net/mlx5: sync stop/start of datapath with secondary process Yongseok Koh
2019-03-25 19:15 ` [dpdk-dev] [PATCH v2 0/4] net/mlx5: rework IPC socket and PMD global data init Yongseok Koh
2019-03-25 19:15 ` Yongseok Koh
2019-03-25 19:15 ` [dpdk-dev] [PATCH v2 1/4] net/mlx5: fix memory event on secondary process Yongseok Koh
2019-03-25 19:15 ` Yongseok Koh
2019-03-26 12:28 ` Shahaf Shuler
2019-03-26 12:28 ` Shahaf Shuler
2019-03-25 19:15 ` [dpdk-dev] [PATCH v2 2/4] net/mlx5: replace IPC socket with EAL API Yongseok Koh
2019-03-25 19:15 ` Yongseok Koh
2019-03-26 12:31 ` Shahaf Shuler
2019-03-26 12:31 ` Shahaf Shuler
2019-03-25 19:15 ` [dpdk-dev] [PATCH v2 3/4] net/mlx5: rework PMD global data init Yongseok Koh
2019-03-25 19:15 ` Yongseok Koh
2019-03-26 12:38 ` Shahaf Shuler
2019-03-26 12:38 ` Shahaf Shuler
2019-03-25 19:15 ` [dpdk-dev] [PATCH v2 4/4] net/mlx5: sync stop/start of datapath with secondary process Yongseok Koh
2019-03-25 19:15 ` Yongseok Koh
2019-03-26 12:49 ` Shahaf Shuler
2019-03-26 12:49 ` Shahaf Shuler
2019-04-01 21:12 ` [dpdk-dev] [PATCH v3 0/4] net/mlx5: rework IPC socket and PMD global data init Yongseok Koh
2019-04-01 21:12 ` Yongseok Koh
2019-04-01 21:12 ` [dpdk-dev] [PATCH v3 1/4] net/mlx5: fix memory event on secondary process Yongseok Koh
2019-04-01 21:12 ` Yongseok Koh
2019-04-01 21:12 ` [dpdk-dev] [PATCH v3 2/4] net/mlx5: replace IPC socket with EAL API Yongseok Koh
2019-04-01 21:12 ` Yongseok Koh
2019-04-01 21:12 ` [dpdk-dev] [PATCH v3 3/4] net/mlx5: rework PMD global data init Yongseok Koh
2019-04-01 21:12 ` Yongseok Koh
2019-04-01 21:12 ` [dpdk-dev] [PATCH v3 4/4] net/mlx5: sync stop/start of datapath with secondary process Yongseok Koh
2019-04-01 21:12 ` Yongseok Koh
2019-04-02 7:11 ` [dpdk-dev] [PATCH v3 0/4] net/mlx5: rework IPC socket and PMD global data init Shahaf Shuler
2019-04-02 7:11 ` Shahaf Shuler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190318212943.GB37866@yongseok-MBP.local \
--to=yskoh@mellanox.com \
--cc=dev@dpdk.org \
--cc=shahafs@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).