From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Tan, Jianfeng" Subject: Re: [PATCH 3/4] drivers/net: do not allocate rte_eth_dev_data privately Date: Tue, 6 Mar 2018 08:55:55 +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: "Richardson, Bruce" , "Ananyev, Konstantin" , Thomas Monjalon , "maxime.coquelin@redhat.com" , "Burakov, Anatoly" , "dev@dpdk.org" To: Matan Azrad , "Yigit, Ferruh" Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id DD5F923D for ; Tue, 6 Mar 2018 09:56:00 +0100 (CET) In-Reply-To: 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" > -----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 >=20 > Hi Jianfeng >=20 > Please see a comment below. >=20 > > 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 mult= i- > > 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 th= e > > pre-defined array so that we can find it. > > > > 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(-) > > > > 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 =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; > > > > 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; > > > > 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); > > > > @@ -775,14 +767,13 @@ rte_pmd_init_internals(struct rte_vdev_device > > *dev, > > > > (*internals)->nb_queues =3D nb_queues; > > > > - 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; > > > > - (*eth_dev)->data =3D data; > > (*eth_dev)->dev_ops =3D &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; > > } > > >=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. Ah, yes, you are correct. I will fix that in v2. >=20 > 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