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

[Matan Azrad <matan@mellanox.com>]

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