From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matan Azrad Subject: Re: [PATCH 3/4] drivers/net: do not allocate rte_eth_dev_data privately Date: Tue, 6 Mar 2018 06:07:39 +0000 Message-ID: References: <1520177405-59091-1-git-send-email-jianfeng.tan@intel.com> <1520177405-59091-4-git-send-email-jianfeng.tan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "bruce.richardson@intel.com" , "konstantin.ananyev@intel.com" , "Thomas Monjalon" , "maxime.coquelin@redhat.com" , "anatoly.burakov@intel.com" , "dev@dpdk.org" To: Jianfeng Tan , "ferruh.yigit@intel.com" Return-path: Received: from EUR02-HE1-obe.outbound.protection.outlook.com (mail-eopbgr10061.outbound.protection.outlook.com [40.107.1.61]) by dpdk.org (Postfix) with ESMTP id F234EE5D for ; Tue, 6 Mar 2018 07:07:41 +0100 (CET) In-Reply-To: <1520177405-59091-4-git-send-email-jianfeng.tan@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Jianfeng Please see a comment below. > From: Jianfeng Tan, Sent: Sunday, March 4, 2018 5:30 PM > We introduced private rte_eth_dev_data to allow vdev to be created both i= n > primary process and secondary process(es). This is not friendly to multi- > process model, for example, it leads to port id contention issue if two > processes both find the data entry is free. >=20 > And to get stats of primary vdev in secondary, we must allocate from the > pre-defined array so that we can find it. >=20 > Suggested-by: Bruce Richardson > Signed-off-by: Jianfeng Tan > --- > drivers/net/af_packet/rte_eth_af_packet.c | 25 +++++++------------------ > drivers/net/kni/rte_eth_kni.c | 13 ++----------- > drivers/net/null/rte_eth_null.c | 17 +++-------------- > drivers/net/octeontx/octeontx_ethdev.c | 14 ++------------ > drivers/net/pcap/rte_eth_pcap.c | 18 +++--------------- > drivers/net/tap/rte_eth_tap.c | 9 +-------- > drivers/net/vhost/rte_eth_vhost.c | 17 ++--------------- > 7 files changed, 20 insertions(+), 93 deletions(-) >=20 > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c > b/drivers/net/af_packet/rte_eth_af_packet.c > index 57eccfd..2db692f 100644 > --- a/drivers/net/af_packet/rte_eth_af_packet.c > +++ b/drivers/net/af_packet/rte_eth_af_packet.c > @@ -564,25 +564,17 @@ rte_pmd_init_internals(struct rte_vdev_device > *dev, > RTE_LOG(ERR, PMD, > "%s: no interface specified for AF_PACKET > ethdev\n", > name); > - goto error_early; > + return -1; > } >=20 > RTE_LOG(INFO, PMD, > "%s: creating AF_PACKET-backed ethdev on numa socket > %u\n", > name, numa_node); >=20 > - /* > - * now do all data allocation - for eth_dev structure, dummy pci > driver > - * and internal (private) data > - */ > - data =3D rte_zmalloc_socket(name, sizeof(*data), 0, numa_node); > - if (data =3D=3D NULL) > - goto error_early; > - > *internals =3D rte_zmalloc_socket(name, sizeof(**internals), > 0, numa_node); > if (*internals =3D=3D NULL) > - goto error_early; > + return -1; >=20 > for (q =3D 0; q < nb_queues; q++) { > (*internals)->rx_queue[q].map =3D MAP_FAILED; @@ -604,24 > +596,24 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > RTE_LOG(ERR, PMD, > "%s: I/F name too long (%s)\n", > name, pair->value); > - goto error_early; > + return -1; > } > if (ioctl(sockfd, SIOCGIFINDEX, &ifr) =3D=3D -1) { > RTE_LOG(ERR, PMD, > "%s: ioctl failed (SIOCGIFINDEX)\n", > name); > - goto error_early; > + return -1; > } > (*internals)->if_name =3D strdup(pair->value); > if ((*internals)->if_name =3D=3D NULL) > - goto error_early; > + return -1; > (*internals)->if_index =3D ifr.ifr_ifindex; >=20 > if (ioctl(sockfd, SIOCGIFHWADDR, &ifr) =3D=3D -1) { > RTE_LOG(ERR, PMD, > "%s: ioctl failed (SIOCGIFHWADDR)\n", > name); > - goto error_early; > + return -1; > } > memcpy(&(*internals)->eth_addr, ifr.ifr_hwaddr.sa_data, > ETH_ALEN); >=20 > @@ -775,14 +767,13 @@ rte_pmd_init_internals(struct rte_vdev_device > *dev, >=20 > (*internals)->nb_queues =3D nb_queues; >=20 > - rte_memcpy(data, (*eth_dev)->data, sizeof(*data)); > + data =3D (*eth_dev)->data; > data->dev_private =3D *internals; > data->nb_rx_queues =3D (uint16_t)nb_queues; > data->nb_tx_queues =3D (uint16_t)nb_queues; > data->dev_link =3D pmd_link; > data->mac_addrs =3D &(*internals)->eth_addr; >=20 > - (*eth_dev)->data =3D data; > (*eth_dev)->dev_ops =3D &ops; >=20 > return 0; > @@ -802,8 +793,6 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > } > free((*internals)->if_name); > rte_free(*internals); > -error_early: > - rte_free(data); > return -1; > } >=20 I think you should remove the private rte_eth_dev_data freeing in rte_pmd_= af_packet_remove(). This is relevant to all the vdevs here. Question: Does the patch include all the vdevs which allocated private rte_eth_dev_da= ta? If so, it may solve also part of the issue discussed here: https://dpdk.org/dev/patchwork/patch/34047/ Matan. > diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.= c > index dc4e65f..1a07089 100644 > --- a/drivers/net/kni/rte_eth_kni.c > +++ b/drivers/net/kni/rte_eth_kni.c > @@ -337,25 +337,17 @@ eth_kni_create(struct rte_vdev_device *vdev, > struct pmd_internals *internals; > struct rte_eth_dev_data *data; > struct rte_eth_dev *eth_dev; > - const char *name; >=20 > RTE_LOG(INFO, PMD, "Creating kni ethdev on numa socket %u\n", > numa_node); >=20 > - name =3D rte_vdev_device_name(vdev); > - data =3D rte_zmalloc_socket(name, sizeof(*data), 0, numa_node); > - if (data =3D=3D NULL) > - return NULL; > - > /* reserve an ethdev entry */ > eth_dev =3D rte_eth_vdev_allocate(vdev, sizeof(*internals)); > - if (eth_dev =3D=3D NULL) { > - rte_free(data); > + if (eth_dev =3D=3D NULL) > return NULL; > - } >=20 > internals =3D eth_dev->data->dev_private; > - rte_memcpy(data, eth_dev->data, sizeof(*data)); > + data =3D eth_dev->data; > data->nb_rx_queues =3D 1; > data->nb_tx_queues =3D 1; > data->dev_link =3D pmd_link; > @@ -363,7 +355,6 @@ eth_kni_create(struct rte_vdev_device *vdev, >=20 > eth_random_addr(internals->eth_addr.addr_bytes); >=20 > - eth_dev->data =3D data; > eth_dev->dev_ops =3D ð_kni_ops; >=20 > internals->no_request_thread =3D args->no_request_thread; diff --git > a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c index > d003b28..98fc60c 100644 > --- a/drivers/net/null/rte_eth_null.c > +++ b/drivers/net/null/rte_eth_null.c > @@ -496,7 +496,7 @@ eth_dev_null_create(struct rte_vdev_device *dev, { > const unsigned nb_rx_queues =3D 1; > const unsigned nb_tx_queues =3D 1; > - struct rte_eth_dev_data *data =3D NULL; > + struct rte_eth_dev_data *data; > struct pmd_internals *internals =3D NULL; > struct rte_eth_dev *eth_dev =3D NULL; >=20 > @@ -513,19 +513,9 @@ eth_dev_null_create(struct rte_vdev_device *dev, > RTE_LOG(INFO, PMD, "Creating null ethdev on numa socket %u\n", > dev->device.numa_node); >=20 > - /* now do all data allocation - for eth_dev structure, dummy pci > driver > - * and internal (private) data > - */ > - data =3D rte_zmalloc_socket(rte_vdev_device_name(dev), > sizeof(*data), 0, > - dev->device.numa_node); > - if (!data) > - return -ENOMEM; > - > eth_dev =3D rte_eth_vdev_allocate(dev, sizeof(*internals)); > - if (!eth_dev) { > - rte_free(data); > + if (!eth_dev) > return -ENOMEM; > - } >=20 > /* now put it all together > * - store queue data in internals, > @@ -546,13 +536,12 @@ eth_dev_null_create(struct rte_vdev_device *dev, >=20 > rte_memcpy(internals->rss_key, default_rss_key, 40); >=20 > - rte_memcpy(data, eth_dev->data, sizeof(*data)); > + data =3D eth_dev->data; > data->nb_rx_queues =3D (uint16_t)nb_rx_queues; > data->nb_tx_queues =3D (uint16_t)nb_tx_queues; > data->dev_link =3D pmd_link; > data->mac_addrs =3D ð_addr; >=20 > - eth_dev->data =3D data; > eth_dev->dev_ops =3D &ops; >=20 > /* finally assign rx and tx ops */ > diff --git a/drivers/net/octeontx/octeontx_ethdev.c > b/drivers/net/octeontx/octeontx_ethdev.c > index b739c0b..f58f6af 100644 > --- a/drivers/net/octeontx/octeontx_ethdev.c > +++ b/drivers/net/octeontx/octeontx_ethdev.c > @@ -1039,7 +1039,7 @@ octeontx_create(struct rte_vdev_device *dev, int > port, uint8_t evdev, > char octtx_name[OCTEONTX_MAX_NAME_LEN]; > struct octeontx_nic *nic =3D NULL; > struct rte_eth_dev *eth_dev =3D NULL; > - struct rte_eth_dev_data *data =3D NULL; > + struct rte_eth_dev_data *data; > const char *name =3D rte_vdev_device_name(dev); >=20 > PMD_INIT_FUNC_TRACE(); > @@ -1055,13 +1055,6 @@ octeontx_create(struct rte_vdev_device *dev, int > port, uint8_t evdev, > return 0; > } >=20 > - data =3D rte_zmalloc_socket(octtx_name, sizeof(*data), 0, socket_id); > - if (data =3D=3D NULL) { > - octeontx_log_err("failed to allocate devdata"); > - res =3D -ENOMEM; > - goto err; > - } > - > nic =3D rte_zmalloc_socket(octtx_name, sizeof(*nic), 0, socket_id); > if (nic =3D=3D NULL) { > octeontx_log_err("failed to allocate nic structure"); @@ - > 1097,11 +1090,9 @@ octeontx_create(struct rte_vdev_device *dev, int port, > uint8_t evdev, > eth_dev->data->kdrv =3D RTE_KDRV_NONE; > eth_dev->data->numa_node =3D dev->device.numa_node; >=20 > - rte_memcpy(data, (eth_dev)->data, sizeof(*data)); > + data =3D eth_dev->data; > data->dev_private =3D nic; > - > data->port_id =3D eth_dev->data->port_id; > - snprintf(data->name, sizeof(data->name), "%s", eth_dev->data- > >name); >=20 > nic->ev_queues =3D 1; > nic->ev_ports =3D 1; > @@ -1120,7 +1111,6 @@ octeontx_create(struct rte_vdev_device *dev, int > port, uint8_t evdev, > goto err; > } >=20 > - eth_dev->data =3D data; > eth_dev->dev_ops =3D &octeontx_dev_ops; >=20 > /* Finally save ethdev pointer to the NIC structure */ diff --git > a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c > index c1571e1..f9f53ff 100644 > --- a/drivers/net/pcap/rte_eth_pcap.c > +++ b/drivers/net/pcap/rte_eth_pcap.c > @@ -773,27 +773,16 @@ pmd_init_internals(struct rte_vdev_device *vdev, > struct pmd_internals **internals, > struct rte_eth_dev **eth_dev) > { > - struct rte_eth_dev_data *data =3D NULL; > + struct rte_eth_dev_data *data; > unsigned int numa_node =3D vdev->device.numa_node; > - const char *name; >=20 > - name =3D rte_vdev_device_name(vdev); > RTE_LOG(INFO, PMD, "Creating pcap-backed ethdev on numa socket > %d\n", > numa_node); >=20 > - /* now do all data allocation - for eth_dev structure > - * and internal (private) data > - */ > - data =3D rte_zmalloc_socket(name, sizeof(*data), 0, numa_node); > - if (data =3D=3D NULL) > - return -1; > - > /* reserve an ethdev entry */ > *eth_dev =3D rte_eth_vdev_allocate(vdev, sizeof(**internals)); > - if (*eth_dev =3D=3D NULL) { > - rte_free(data); > + if (*eth_dev =3D=3D NULL) > return -1; > - } >=20 > /* now put it all together > * - store queue data in internals, > @@ -802,7 +791,7 @@ pmd_init_internals(struct rte_vdev_device *vdev, > * - and point eth_dev structure to new eth_dev_data structure > */ > *internals =3D (*eth_dev)->data->dev_private; > - rte_memcpy(data, (*eth_dev)->data, sizeof(*data)); > + data =3D (*eth_dev)->data; > data->nb_rx_queues =3D (uint16_t)nb_rx_queues; > data->nb_tx_queues =3D (uint16_t)nb_tx_queues; > data->dev_link =3D pmd_link; > @@ -812,7 +801,6 @@ pmd_init_internals(struct rte_vdev_device *vdev, > * NOTE: we'll replace the data element, of originally allocated > * eth_dev so the rings are local per-process > */ > - (*eth_dev)->data =3D data; > (*eth_dev)->dev_ops =3D &ops; >=20 > return 0; > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.= c > index f09db0e..0fb8be5 100644 > --- a/drivers/net/tap/rte_eth_tap.c > +++ b/drivers/net/tap/rte_eth_tap.c > @@ -1348,12 +1348,6 @@ eth_dev_tap_create(struct rte_vdev_device > *vdev, char *tap_name, >=20 > RTE_LOG(DEBUG, PMD, " TAP device on numa %u\n", > rte_socket_id()); >=20 > - data =3D rte_zmalloc_socket(tap_name, sizeof(*data), 0, numa_node); > - if (!data) { > - RTE_LOG(ERR, PMD, "TAP Failed to allocate data\n"); > - goto error_exit_nodev; > - } > - > dev =3D rte_eth_vdev_allocate(vdev, sizeof(*pmd)); > if (!dev) { > RTE_LOG(ERR, PMD, "TAP Unable to allocate device > struct\n"); @@ -1373,7 +1367,7 @@ eth_dev_tap_create(struct > rte_vdev_device *vdev, char *tap_name, > } >=20 > /* Setup some default values */ > - rte_memcpy(data, dev->data, sizeof(*data)); > + data =3D dev->data; > data->dev_private =3D pmd; > data->dev_flags =3D RTE_ETH_DEV_INTR_LSC; > data->numa_node =3D numa_node; > @@ -1384,7 +1378,6 @@ eth_dev_tap_create(struct rte_vdev_device > *vdev, char *tap_name, > data->nb_rx_queues =3D 0; > data->nb_tx_queues =3D 0; >=20 > - dev->data =3D data; > dev->dev_ops =3D &ops; > dev->rx_pkt_burst =3D pmd_rx_burst; > dev->tx_pkt_burst =3D pmd_tx_burst; > diff --git a/drivers/net/vhost/rte_eth_vhost.c > b/drivers/net/vhost/rte_eth_vhost.c > index 3aae01c..aa06ab5 100644 > --- a/drivers/net/vhost/rte_eth_vhost.c > +++ b/drivers/net/vhost/rte_eth_vhost.c > @@ -1016,7 +1016,7 @@ eth_dev_vhost_create(struct rte_vdev_device > *dev, char *iface_name, > int16_t queues, const unsigned int numa_node, uint64_t flags) { > const char *name =3D rte_vdev_device_name(dev); > - struct rte_eth_dev_data *data =3D NULL; > + struct rte_eth_dev_data *data; > struct pmd_internal *internal =3D NULL; > struct rte_eth_dev *eth_dev =3D NULL; > struct ether_addr *eth_addr =3D NULL; > @@ -1026,13 +1026,6 @@ eth_dev_vhost_create(struct rte_vdev_device > *dev, char *iface_name, > RTE_LOG(INFO, PMD, "Creating VHOST-USER backend on numa > socket %u\n", > numa_node); >=20 > - /* now do all data allocation - for eth_dev structure and internal > - * (private) data > - */ > - data =3D rte_zmalloc_socket(name, sizeof(*data), 0, numa_node); > - if (data =3D=3D NULL) > - goto error; > - > list =3D rte_zmalloc_socket(name, sizeof(*list), 0, numa_node); > if (list =3D=3D NULL) > goto error; > @@ -1074,12 +1067,7 @@ eth_dev_vhost_create(struct rte_vdev_device > *dev, char *iface_name, > rte_spinlock_init(&vring_state->lock); > vring_states[eth_dev->data->port_id] =3D vring_state; >=20 > - /* We'll replace the 'data' originally allocated by eth_dev. So the > - * vhost PMD resources won't be shared between multi processes. > - */ > - rte_memcpy(data, eth_dev->data, sizeof(*data)); > - eth_dev->data =3D data; > - > + data =3D eth_dev->data; > data->nb_rx_queues =3D queues; > data->nb_tx_queues =3D queues; > internal->max_queues =3D queues; > @@ -1120,7 +1108,6 @@ eth_dev_vhost_create(struct rte_vdev_device > *dev, char *iface_name, > rte_eth_dev_release_port(eth_dev); > rte_free(internal); > rte_free(list); > - rte_free(data); >=20 > return -1; > } > -- > 2.7.4