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 in
> 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.
>
> And to get stats of primary vdev in secondary, we must allocate from the
> pre-defined array so that we can find it.
>
> Suggested-by: Bruce Richardson <bruce.richard...@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng....@intel.com>
> ---
> 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(-)
>
> 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;
> }
>
> RTE_LOG(INFO, PMD,
> "%s: creating AF_PACKET-backed ethdev on numa socket
> %u\n",
> name, numa_node);
>
> - /*
> - * now do all data allocation - for eth_dev structure, dummy pci
> driver
> - * and internal (private) data
> - */
> - data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
> - if (data == NULL)
> - goto error_early;
> -
> *internals = rte_zmalloc_socket(name, sizeof(**internals),
> 0, numa_node);
> if (*internals == NULL)
> - goto error_early;
> + return -1;
>
> for (q = 0; q < nb_queues; q++) {
> (*internals)->rx_queue[q].map = 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) == -1) {
> RTE_LOG(ERR, PMD,
> "%s: ioctl failed (SIOCGIFINDEX)\n",
> name);
> - goto error_early;
> + return -1;
> }
> (*internals)->if_name = strdup(pair->value);
> if ((*internals)->if_name == NULL)
> - goto error_early;
> + return -1;
> (*internals)->if_index = ifr.ifr_ifindex;
>
> if (ioctl(sockfd, SIOCGIFHWADDR, &ifr) == -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);
>
> @@ -775,14 +767,13 @@ rte_pmd_init_internals(struct rte_vdev_device
> *dev,
>
> (*internals)->nb_queues = nb_queues;
>
> - rte_memcpy(data, (*eth_dev)->data, sizeof(*data));
> + data = (*eth_dev)->data;
> data->dev_private = *internals;
> data->nb_rx_queues = (uint16_t)nb_queues;
> data->nb_tx_queues = (uint16_t)nb_queues;
> data->dev_link = pmd_link;
> data->mac_addrs = &(*internals)->eth_addr;
>
> - (*eth_dev)->data = data;
> (*eth_dev)->dev_ops = &ops;
>
> 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;
> }
>
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_data?
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;
>
> RTE_LOG(INFO, PMD, "Creating kni ethdev on numa socket %u\n",
> numa_node);
>
> - name = rte_vdev_device_name(vdev);
> - data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
> - if (data == NULL)
> - return NULL;
> -
> /* reserve an ethdev entry */
> eth_dev = rte_eth_vdev_allocate(vdev, sizeof(*internals));
> - if (eth_dev == NULL) {
> - rte_free(data);
> + if (eth_dev == NULL)
> return NULL;
> - }
>
> internals = eth_dev->data->dev_private;
> - rte_memcpy(data, eth_dev->data, sizeof(*data));
> + data = eth_dev->data;
> data->nb_rx_queues = 1;
> data->nb_tx_queues = 1;
> data->dev_link = pmd_link;
> @@ -363,7 +355,6 @@ eth_kni_create(struct rte_vdev_device *vdev,
>
> eth_random_addr(internals->eth_addr.addr_bytes);
>
> - eth_dev->data = data;
> eth_dev->dev_ops = ð_kni_ops;
>
> internals->no_request_thread = 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 = 1;
> const unsigned nb_tx_queues = 1;
> - struct rte_eth_dev_data *data = NULL;
> + struct rte_eth_dev_data *data;
> struct pmd_internals *internals = NULL;
> struct rte_eth_dev *eth_dev = NULL;
>
> @@ -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);
>
> - /* now do all data allocation - for eth_dev structure, dummy pci
> driver
> - * and internal (private) data
> - */
> - data = rte_zmalloc_socket(rte_vdev_device_name(dev),
> sizeof(*data), 0,
> - dev->device.numa_node);
> - if (!data)
> - return -ENOMEM;
> -
> eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internals));
> - if (!eth_dev) {
> - rte_free(data);
> + if (!eth_dev)
> return -ENOMEM;
> - }
>
> /* now put it all together
> * - store queue data in internals,
> @@ -546,13 +536,12 @@ eth_dev_null_create(struct rte_vdev_device *dev,
>
> rte_memcpy(internals->rss_key, default_rss_key, 40);
>
> - rte_memcpy(data, eth_dev->data, sizeof(*data));
> + data = eth_dev->data;
> data->nb_rx_queues = (uint16_t)nb_rx_queues;
> data->nb_tx_queues = (uint16_t)nb_tx_queues;
> data->dev_link = pmd_link;
> data->mac_addrs = ð_addr;
>
> - eth_dev->data = data;
> eth_dev->dev_ops = &ops;
>
> /* 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 = NULL;
> struct rte_eth_dev *eth_dev = NULL;
> - struct rte_eth_dev_data *data = NULL;
> + struct rte_eth_dev_data *data;
> const char *name = rte_vdev_device_name(dev);
>
> PMD_INIT_FUNC_TRACE();
> @@ -1055,13 +1055,6 @@ octeontx_create(struct rte_vdev_device *dev, int
> port, uint8_t evdev,
> return 0;
> }
>
> - data = rte_zmalloc_socket(octtx_name, sizeof(*data), 0, socket_id);
> - if (data == NULL) {
> - octeontx_log_err("failed to allocate devdata");
> - res = -ENOMEM;
> - goto err;
> - }
> -
> nic = rte_zmalloc_socket(octtx_name, sizeof(*nic), 0, socket_id);
> if (nic == 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 = RTE_KDRV_NONE;
> eth_dev->data->numa_node = dev->device.numa_node;
>
> - rte_memcpy(data, (eth_dev)->data, sizeof(*data));
> + data = eth_dev->data;
> data->dev_private = nic;
> -
> data->port_id = eth_dev->data->port_id;
> - snprintf(data->name, sizeof(data->name), "%s", eth_dev->data-
> >name);
>
> nic->ev_queues = 1;
> nic->ev_ports = 1;
> @@ -1120,7 +1111,6 @@ octeontx_create(struct rte_vdev_device *dev, int
> port, uint8_t evdev,
> goto err;
> }
>
> - eth_dev->data = data;
> eth_dev->dev_ops = &octeontx_dev_ops;
>
> /* 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 = NULL;
> + struct rte_eth_dev_data *data;
> unsigned int numa_node = vdev->device.numa_node;
> - const char *name;
>
> - name = rte_vdev_device_name(vdev);
> RTE_LOG(INFO, PMD, "Creating pcap-backed ethdev on numa socket
> %d\n",
> numa_node);
>
> - /* now do all data allocation - for eth_dev structure
> - * and internal (private) data
> - */
> - data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
> - if (data == NULL)
> - return -1;
> -
> /* reserve an ethdev entry */
> *eth_dev = rte_eth_vdev_allocate(vdev, sizeof(**internals));
> - if (*eth_dev == NULL) {
> - rte_free(data);
> + if (*eth_dev == NULL)
> return -1;
> - }
>
> /* 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 = (*eth_dev)->data->dev_private;
> - rte_memcpy(data, (*eth_dev)->data, sizeof(*data));
> + data = (*eth_dev)->data;
> data->nb_rx_queues = (uint16_t)nb_rx_queues;
> data->nb_tx_queues = (uint16_t)nb_tx_queues;
> data->dev_link = 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 = data;
> (*eth_dev)->dev_ops = &ops;
>
> 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,
>
> RTE_LOG(DEBUG, PMD, " TAP device on numa %u\n",
> rte_socket_id());
>
> - data = 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 = 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,
> }
>
> /* Setup some default values */
> - rte_memcpy(data, dev->data, sizeof(*data));
> + data = dev->data;
> data->dev_private = pmd;
> data->dev_flags = RTE_ETH_DEV_INTR_LSC;
> data->numa_node = numa_node;
> @@ -1384,7 +1378,6 @@ eth_dev_tap_create(struct rte_vdev_device
> *vdev, char *tap_name,
> data->nb_rx_queues = 0;
> data->nb_tx_queues = 0;
>
> - dev->data = data;
> dev->dev_ops = &ops;
> dev->rx_pkt_burst = pmd_rx_burst;
> dev->tx_pkt_burst = 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 = rte_vdev_device_name(dev);
> - struct rte_eth_dev_data *data = NULL;
> + struct rte_eth_dev_data *data;
> struct pmd_internal *internal = NULL;
> struct rte_eth_dev *eth_dev = NULL;
> struct ether_addr *eth_addr = 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);
>
> - /* now do all data allocation - for eth_dev structure and internal
> - * (private) data
> - */
> - data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
> - if (data == NULL)
> - goto error;
> -
> list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
> if (list == 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] = vring_state;
>
> - /* 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 = data;
> -
> + data = eth_dev->data;
> data->nb_rx_queues = queues;
> data->nb_tx_queues = queues;
> internal->max_queues = 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);
>
> return -1;
> }
> --
> 2.7.4
