[dpdk-dev] [PATCH 3/4] drivers/net: do not allocate rte_eth_dev_data privately
Tan, Jianfeng
jianfeng.tan at intel.com
Tue Mar 6 09:55:55 CET 2018
> -----Original Message-----
> From: Matan Azrad [mailto:matan at mellanox.com]
> Sent: Tuesday, March 6, 2018 2:08 PM
> To: Tan, Jianfeng; Yigit, Ferruh
> Cc: Richardson, Bruce; Ananyev, Konstantin; Thomas Monjalon;
> maxime.coquelin at redhat.com; Burakov, Anatoly; dev at 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 at intel.com>
> > Signed-off-by: Jianfeng Tan <jianfeng.tan at 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
More information about the dev
mailing list