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:ma...@mellanox.com] > > > Sent: Tuesday, March 6, 2018 2:08 PM > > > To: Tan, Jianfeng; Yigit, Ferruh > > > Cc: Richardson, Bruce; Ananyev, Konstantin; Thomas Monjalon; > > > maxime.coque...@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.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. > > > > 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
