From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH 2/4] net/mlx5: replace IPC socket with EAL API Date: Mon, 18 Mar 2019 21:29:53 +0000 Message-ID: <20190318212943.GB37866@yongseok-MBP.local> References: <20190307073314.18324-1-yskoh@mellanox.com> <20190307073314.18324-3-yskoh@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Shahaf Shuler Return-path: Received: from EUR04-DB3-obe.outbound.protection.outlook.com (mail-eopbgr60063.outbound.protection.outlook.com [40.107.6.63]) by dpdk.org (Postfix) with ESMTP id 5FA9C239 for ; Mon, 18 Mar 2019 22:29:55 +0100 (CET) In-Reply-To: Content-Language: en-US Content-ID: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Mar 14, 2019 at 05:36:32AM -0700, Shahaf Shuler wrote: > Hi Koh,=20 >=20 > Thursday, March 7, 2019 9:33 AM, Yongseok Koh: > > Subject: [PATCH 2/4] net/mlx5: replace IPC socket with EAL API > >=20 > > Socket API is used for IPC in order for secondary process to acquire Ve= rb > > 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. > >=20 > > 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 n= eed > > to reference a specific device. > >=20 > > Signed-off-by: Yongseok Koh >=20 > [...] >=20 > > /** > > diff --git a/drivers/net/mlx5/mlx5_mp.c b/drivers/net/mlx5/mlx5_mp.c ne= w > > 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 > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > + > > +#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 =3D (struct mlx5_mp_param *)msg- > > >param; > > + > > + memset(msg, 0, sizeof(*msg)); > > + strlcpy(msg->name, MLX5_MP_NAME, sizeof(msg->name)); > > + msg->len_param =3D sizeof(*param); > > + param->type =3D type; > > + param->port_id =3D 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 s= et. > > + */ > > +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 =3D (struct mlx5_mp_param > > *)mp_res.param; > > + const struct mlx5_mp_param *param =3D > > + (const struct mlx5_mp_param *)mp_msg->param; > > + struct rte_eth_dev *dev =3D &rte_eth_devices[param->port_id]; >=20 > Need to check dev is a valid one before we continue. If such case happen = need error log (like you have for invalid req).=20 Good point. > > + struct mlx5_priv *priv =3D dev->data->dev_private; > > + int ret =3D 0; > > + > > + assert(rte_eal_process_type() =3D=3D 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 =3D 1; > > + mp_res.fds[0] =3D priv->ctx->cmd_fd; > > + res->result =3D 0; > > + ret =3D rte_mp_reply(&mp_res, peer); > > + break; > > + default: > > + rte_errno =3D 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 proce= ss. > > + * > > + * @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 =3D {.tv_sec =3D 5, .tv_nsec =3D 0}; > > + int cmd_fd; > > + int ret; > > + > > + assert(rte_eal_process_type() =3D=3D RTE_PROC_SECONDARY); > > + mp_init_msg(dev, &mp_req, MLX5_MP_REQ_VERBS_CMD_FD); > > + ret =3D 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 =3D=3D 1); > > + mp_res =3D &mp_rep.msgs[0]; > > + res =3D (struct mlx5_mp_param *)mp_res->param; > > + assert(!res->result); >=20 > Above should not be an assert rather and actual check.=20 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 =3D=3D 1); > > + cmd_fd =3D 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() =3D=3D 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 > > -#include > > -#include > > -#include > > -#include > > -#include > > -#include > > - > > -#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 s= et. > > - */ > > -int > > -mlx5_socket_init(struct rte_eth_dev *dev) -{ > > - struct mlx5_priv *priv =3D dev->data->dev_private; > > - struct sockaddr_un sun =3D { > > - .sun_family =3D 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 =3D socket(AF_UNIX, SOCK_STREAM, 0); > > - if (ret < 0) { > > - rte_errno =3D errno; > > - DRV_LOG(WARNING, "port %u secondary process not > > supported: %s", > > - dev->data->port_id, strerror(errno)); > > - goto error; > > - } > > - priv->primary_socket =3D ret; > > - flags =3D fcntl(priv->primary_socket, F_GETFL, 0); > > - if (flags =3D=3D -1) { > > - rte_errno =3D errno; > > - goto error; > > - } > > - ret =3D fcntl(priv->primary_socket, F_SETFL, flags | O_NONBLOCK); > > - if (ret < 0) { > > - rte_errno =3D 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 =3D bind(priv->primary_socket, (const struct sockaddr *)&sun, > > - sizeof(sun)); > > - if (ret < 0) { > > - rte_errno =3D errno; > > - DRV_LOG(WARNING, > > - "port %u cannot bind socket, secondary process not" > > - " supported: %s", > > - dev->data->port_id, strerror(errno)); > > - goto close; > > - } > > - ret =3D listen(priv->primary_socket, 0); > > - if (ret < 0) { > > - rte_errno =3D 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 =3D 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 =3D 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 =3D 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 =3D dev->data->dev_private; > > - int conn_sock; > > - int ret =3D 0; > > - struct cmsghdr *cmsg =3D NULL; > > - struct ucred *cred =3D NULL; > > - char buf[CMSG_SPACE(sizeof(struct ucred))] =3D { 0 }; > > - char vbuf[1024] =3D { 0 }; > > - struct iovec io =3D { > > - .iov_base =3D vbuf, > > - .iov_len =3D sizeof(*vbuf), > > - }; > > - struct msghdr msg =3D { > > - .msg_iov =3D &io, > > - .msg_iovlen =3D 1, > > - .msg_control =3D buf, > > - .msg_controllen =3D sizeof(buf), > > - }; > > - int *fd; > > - > > - /* Accept the connection from the client. */ > > - conn_sock =3D 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 =3D setsockopt(conn_sock, SOL_SOCKET, SO_PASSCRED, &(int){1}, > > - sizeof(int)); > > - if (ret < 0) { > > - ret =3D errno; > > - DRV_LOG(WARNING, "port %u cannot change socket > > options: %s", > > - dev->data->port_id, strerror(rte_errno)); > > - goto error; > > - } > > - ret =3D recvmsg(conn_sock, &msg, MSG_WAITALL); > > - if (ret < 0) { > > - ret =3D 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 =3D CMSG_FIRSTHDR(&msg); > > - if (cmsg =3D=3D NULL) { > > - DRV_LOG(WARNING, "port %u no message", dev->data- > > >port_id); > > - goto error; > > - } > > - if ((cmsg->cmsg_type =3D=3D SCM_CREDENTIALS) && > > - (cmsg->cmsg_len >=3D sizeof(*cred))) { > > - cred =3D (struct ucred *)CMSG_DATA(cmsg); > > - assert(cred !=3D NULL); > > - } > > - cmsg =3D CMSG_NXTHDR(&msg, cmsg); > > - if (cmsg !=3D 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 =3D=3D NULL) || (cred->uid !=3D getuid()) || > > - (cred->gid !=3D getgid())) { > > - DRV_LOG(WARNING, "port %u wrong credentials", > > - dev->data->port_id); > > - goto error; > > - } > > - /* Set-up the ancillary data. */ > > - cmsg =3D CMSG_FIRSTHDR(&msg); > > - assert(cmsg !=3D NULL); > > - cmsg->cmsg_level =3D SOL_SOCKET; > > - cmsg->cmsg_type =3D SCM_RIGHTS; > > - cmsg->cmsg_len =3D CMSG_LEN(sizeof(priv->ctx->cmd_fd)); > > - fd =3D (int *)CMSG_DATA(cmsg); > > - *fd =3D priv->ctx->cmd_fd; > > - ret =3D 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 se= t. > > - */ > > -int > > -mlx5_socket_connect(struct rte_eth_dev *dev) -{ > > - struct mlx5_priv *priv =3D dev->data->dev_private; > > - struct sockaddr_un sun =3D { > > - .sun_family =3D AF_UNIX, > > - }; > > - int socket_fd =3D -1; > > - int *fd =3D NULL; > > - int ret; > > - struct ucred *cred; > > - char buf[CMSG_SPACE(sizeof(*cred))] =3D { 0 }; > > - char vbuf[1024] =3D { 0 }; > > - struct iovec io =3D { > > - .iov_base =3D vbuf, > > - .iov_len =3D sizeof(*vbuf), > > - }; > > - struct msghdr msg =3D { > > - .msg_control =3D buf, > > - .msg_controllen =3D sizeof(buf), > > - .msg_iov =3D &io, > > - .msg_iovlen =3D 1, > > - }; > > - struct cmsghdr *cmsg; > > - > > - ret =3D socket(AF_UNIX, SOCK_STREAM, 0); > > - if (ret < 0) { > > - rte_errno =3D errno; > > - DRV_LOG(WARNING, "port %u cannot connect to primary", > > - dev->data->port_id); > > - goto error; > > - } > > - socket_fd =3D ret; > > - snprintf(sun.sun_path, sizeof(sun.sun_path), "/var/tmp/%s_%d", > > - MLX5_DRIVER_NAME, priv->primary_socket); > > - ret =3D connect(socket_fd, (const struct sockaddr *)&sun, sizeof(sun)= ); > > - if (ret < 0) { > > - rte_errno =3D errno; > > - DRV_LOG(WARNING, "port %u cannot connect to primary", > > - dev->data->port_id); > > - goto error; > > - } > > - cmsg =3D CMSG_FIRSTHDR(&msg); > > - if (cmsg =3D=3D NULL) { > > - rte_errno =3D EINVAL; > > - DRV_LOG(DEBUG, "port %u cannot get first message", > > - dev->data->port_id); > > - goto error; > > - } > > - cmsg->cmsg_level =3D SOL_SOCKET; > > - cmsg->cmsg_type =3D SCM_CREDENTIALS; > > - cmsg->cmsg_len =3D CMSG_LEN(sizeof(*cred)); > > - cred =3D (struct ucred *)CMSG_DATA(cmsg); > > - if (cred =3D=3D NULL) { > > - rte_errno =3D EINVAL; > > - DRV_LOG(DEBUG, "port %u no credentials received", > > - dev->data->port_id); > > - goto error; > > - } > > - cred->pid =3D getpid(); > > - cred->uid =3D getuid(); > > - cred->gid =3D getgid(); > > - ret =3D sendmsg(socket_fd, &msg, MSG_DONTWAIT); > > - if (ret < 0) { > > - rte_errno =3D errno; > > - DRV_LOG(WARNING, > > - "port %u cannot send credentials to primary: %s", > > - dev->data->port_id, strerror(errno)); > > - goto error; > > - } > > - ret =3D recvmsg(socket_fd, &msg, MSG_WAITALL); > > - if (ret <=3D 0) { > > - rte_errno =3D errno; > > - DRV_LOG(WARNING, "port %u no message from primary: > > %s", > > - dev->data->port_id, strerror(errno)); > > - goto error; > > - } > > - cmsg =3D CMSG_FIRSTHDR(&msg); > > - if (cmsg =3D=3D NULL) { > > - rte_errno =3D EINVAL; > > - DRV_LOG(WARNING, "port %u no file descriptor received", > > - dev->data->port_id); > > - goto error; > > - } > > - fd =3D (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 =3D *fd; > > - goto error; > > - } > > - ret =3D *fd; > > - close(socket_fd); > > - return ret; > > -error: > > - if (socket_fd !=3D -1) > > - close(socket_fd); > > - return -rte_errno; > > -} > > -- > > 2.11.0 >=20