[dpdk-dev,3/4] drivers/net: do not allocate rte_eth_dev_data privately

Message ID 1520177405-59091-4-git-send-email-jianfeng.tan@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Jianfeng Tan March 4, 2018, 3:30 p.m. UTC
  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.richardson@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@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(-)
  

Comments

Matan Azrad March 6, 2018, 6:07 a.m. UTC | #1
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.richardson@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@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 = &eth_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 = &eth_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
  
Jianfeng Tan March 6, 2018, 8:55 a.m. UTC | #2
> -----Original Message-----
> From: Matan Azrad [mailto:matan@mellanox.com]
> Sent: Tuesday, March 6, 2018 2:08 PM
> To: Tan, Jianfeng; Yigit, Ferruh
> Cc: Richardson, Bruce; Ananyev, Konstantin; Thomas Monjalon;
> maxime.coquelin@redhat.com; Burakov, Anatoly; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 3/4] drivers/net: do not allocate
> rte_eth_dev_data privately
> 
> 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.richardson@intel.com>
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@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.

Ah, yes, you are correct. I will fix that in v2.

> 
> Question:
> Does the patch include all the vdevs which allocated private
> rte_eth_dev_data?

Yes, we are removing all private rte_eth_dev_data. If I missed some device, welcome to point out.

> If so, it may solve also part of the issue discussed here:
> https://dpdk.org/dev/patchwork/patch/34047/

Yes, related. We now allocate rte_eth_dev_data which can be indexed by all primary/secondary processes.

Thanks,
Jianfeng
  
Matan Azrad March 7, 2018, 6 a.m. UTC | #3
Hi Jianfeng

From: Tan, Jianfeng, Sent: Tuesday, March 6, 2018 10:56 AM
> > -----Original Message-----
> > From: Matan Azrad [mailto:matan@mellanox.com]
> > Sent: Tuesday, March 6, 2018 2:08 PM
> > To: Tan, Jianfeng; Yigit, Ferruh
> > Cc: Richardson, Bruce; Ananyev, Konstantin; Thomas Monjalon;
> > maxime.coquelin@redhat.com; Burakov, Anatoly; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH 3/4] drivers/net: do not allocate
> > rte_eth_dev_data privately
> >
> > 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.richardson@intel.com>
> > > Signed-off-by: Jianfeng Tan <jianfeng.tan@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.
> 
> Ah, yes, you are correct. I will fix that in v2.
> 
> >
> > Question:
> > Does the patch include all the vdevs which allocated private
> > rte_eth_dev_data?
> 
> Yes, we are removing all private rte_eth_dev_data. If I missed some device,
> welcome to point out.

net/ring

> > If so, it may solve also part of the issue discussed here:
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdp
> d
> >
> k.org%2Fdev%2Fpatchwork%2Fpatch%2F34047%2F&data=02%7C01%7Cmata
> n%40mell
> >
> anox.com%7C4143e70010774a15672708d583401618%7Ca652971c7d2e4d9ba6
> a4d149
> >
> 256f461b%7C0%7C0%7C636559233645410291&sdata=G1pYHEXENP3low8oziaI
> KsxiHB
> > mlEjV1f89LMZmnzvc%3D&reserved=0
> 
> Yes, related. We now allocate rte_eth_dev_data which can be indexed by all
> primary/secondary processes.
> 
> Thanks,
> Jianfeng
  
Matan Azrad March 7, 2018, 6:10 a.m. UTC | #4
Hi Jianfeng


From: Matan Azrad, Wednesday, March 7, 2018 8:01 AM
> Hi Jianfeng
> 
> From: Tan, Jianfeng, Sent: Tuesday, March 6, 2018 10:56 AM
> > > -----Original Message-----
> > > From: Matan Azrad [mailto:matan@mellanox.com]
> > > Sent: Tuesday, March 6, 2018 2:08 PM
> > > To: Tan, Jianfeng; Yigit, Ferruh
> > > Cc: Richardson, Bruce; Ananyev, Konstantin; Thomas Monjalon;
> > > maxime.coquelin@redhat.com; Burakov, Anatoly; dev@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH 3/4] drivers/net: do not allocate
> > > rte_eth_dev_data privately
> > >
> > > 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.richardson@intel.com>
> > > > Signed-off-by: Jianfeng Tan <jianfeng.tan@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.
> >
> > Ah, yes, you are correct. I will fix that in v2.
> >
> > >
> > > Question:
> > > Does the patch include all the vdevs which allocated private
> > > rte_eth_dev_data?
> >
> > Yes, we are removing all private rte_eth_dev_data. If I missed some
> > device, welcome to point out.
> 
> net/ring
> 

What is about next PCI device?

net/cxgbe

> > > If so, it may solve also part of the issue discussed here:
> > >
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdp
> > d
> > >
> >
> k.org%2Fdev%2Fpatchwork%2Fpatch%2F34047%2F&data=02%7C01%7Cmata
> > n%40mell
> > >
> >
> anox.com%7C4143e70010774a15672708d583401618%7Ca652971c7d2e4d9ba6
> > a4d149
> > >
> >
> 256f461b%7C0%7C0%7C636559233645410291&sdata=G1pYHEXENP3low8oziaI
> > KsxiHB
> > > mlEjV1f89LMZmnzvc%3D&reserved=0
> >
> > Yes, related. We now allocate rte_eth_dev_data which can be indexed by
> > all primary/secondary processes.
> >
> > Thanks,
> > Jianfeng
  
Jianfeng Tan March 12, 2018, 3:40 a.m. UTC | #5
On 3/7/2018 2:10 PM, Matan Azrad wrote:
>
>>> Yes, we are removing all private rte_eth_dev_data. If I missed some
>>> device, welcome to point out.
>> net/ring

>> What is about next PCI device?
>>
>> net/cxgbe

Will change these two in next version. Thank you, Matan.
  

Patch

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;
 }
 
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 = &eth_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 = &eth_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;
 }