From: Matan Azrad <matan@mellanox.com>
To: Jianfeng Tan <jianfeng.tan@intel.com>,
"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>
Cc: "bruce.richardson@intel.com" <bruce.richardson@intel.com>,
"konstantin.ananyev@intel.com" <konstantin.ananyev@intel.com>,
"Thomas Monjalon" <thomas@monjalon.net>,
"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
"anatoly.burakov@intel.com" <anatoly.burakov@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 3/4] drivers/net: do not allocate rte_eth_dev_data privately
Date: Tue, 6 Mar 2018 06:07:39 +0000 [thread overview]
Message-ID: <AM4PR0501MB26578466FF367876668DCE40D2D90@AM4PR0501MB2657.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <1520177405-59091-4-git-send-email-jianfeng.tan@intel.com>
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 = ð_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 = ð_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
next prev parent reply other threads:[~2018-03-06 6:07 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-04 15:30 [dpdk-dev] [PATCH 0/4] allow procinfo and pdump on eth vdev Jianfeng Tan
2018-03-04 15:30 ` [dpdk-dev] [PATCH 1/4] eal: bring forward multi-process channel init Jianfeng Tan
2018-03-04 15:30 ` [dpdk-dev] [PATCH 2/4] bus/vdev: bus scan by multi-process channel Jianfeng Tan
2018-03-05 9:36 ` Burakov, Anatoly
2018-03-06 0:50 ` Tan, Jianfeng
2018-03-07 14:00 ` Burakov, Anatoly
2018-03-12 3:22 ` Tan, Jianfeng
2018-03-04 15:30 ` [dpdk-dev] [PATCH 3/4] drivers/net: do not allocate rte_eth_dev_data privately Jianfeng Tan
2018-03-06 6:07 ` Matan Azrad [this message]
2018-03-06 8:55 ` Tan, Jianfeng
2018-03-07 6:00 ` Matan Azrad
2018-03-07 6:10 ` Matan Azrad
2018-03-12 3:40 ` Tan, Jianfeng
2018-03-04 15:30 ` [dpdk-dev] [PATCH 4/4] drivers/net: share vdev data to secondary process Jianfeng Tan
2018-04-19 16:50 ` [dpdk-dev] [PATCH v3 0/5] allow procinfo and pdump on eth vdev Jianfeng Tan
2018-04-19 16:50 ` [dpdk-dev] [PATCH v3 1/5] eal: bring forward multi-process channel init Jianfeng Tan
2018-04-20 8:16 ` Burakov, Anatoly
2018-04-20 14:08 ` Tan, Jianfeng
2018-04-19 16:50 ` [dpdk-dev] [PATCH v3 2/5] bus/vdev: add lock on vdev device list Jianfeng Tan
2018-04-20 8:26 ` Burakov, Anatoly
2018-04-20 14:19 ` Tan, Jianfeng
2018-04-20 15:16 ` Burakov, Anatoly
2018-04-20 15:23 ` Tan, Jianfeng
2018-04-19 16:50 ` [dpdk-dev] [PATCH v3 3/5] bus/vdev: bus scan by multi-process channel Jianfeng Tan
2018-04-20 8:41 ` Burakov, Anatoly
2018-04-20 14:28 ` Tan, Jianfeng
2018-04-20 15:19 ` Burakov, Anatoly
2018-04-20 15:32 ` Tan, Jianfeng
2018-04-20 15:39 ` Burakov, Anatoly
2018-04-19 16:50 ` [dpdk-dev] [PATCH v3 4/5] drivers/net: not use private eth dev data Jianfeng Tan
2018-04-19 16:50 ` [dpdk-dev] [PATCH v3 5/5] drivers/net: share vdev data to secondary process Jianfeng Tan
2018-04-20 16:57 ` [dpdk-dev] [PATCH v4 0/5] allow procinfo and pdump on eth vdev Jianfeng Tan
2018-04-20 16:57 ` [dpdk-dev] [PATCH v4 1/5] eal: bring forward multi-process channel init Jianfeng Tan
2018-04-20 16:57 ` [dpdk-dev] [PATCH v4 2/5] bus/vdev: add lock on vdev device list Jianfeng Tan
2018-04-23 9:47 ` Burakov, Anatoly
2018-04-20 16:57 ` [dpdk-dev] [PATCH v4 3/5] bus/vdev: bus scan by multi-process channel Jianfeng Tan
2018-04-23 9:54 ` Burakov, Anatoly
2018-04-24 5:22 ` Tan, Jianfeng
2018-04-20 16:57 ` [dpdk-dev] [PATCH v4 4/5] drivers/net: not use private eth dev data Jianfeng Tan
2018-04-20 16:57 ` [dpdk-dev] [PATCH v4 5/5] drivers/net: share vdev data to secondary process Jianfeng Tan
2018-04-24 5:51 ` [dpdk-dev] [PATCH v5 0/5] allow procinfo and pdump on eth vdev Jianfeng Tan
2018-04-24 5:51 ` [dpdk-dev] [PATCH v5 1/5] eal: bring forward multi-process channel init Jianfeng Tan
2018-04-24 5:51 ` [dpdk-dev] [PATCH v5 2/5] bus/vdev: add lock on vdev device list Jianfeng Tan
2018-04-24 5:51 ` [dpdk-dev] [PATCH v5 3/5] bus/vdev: bus scan by multi-process channel Jianfeng Tan
2018-04-24 10:09 ` Thomas Monjalon
2018-04-24 5:51 ` [dpdk-dev] [PATCH v5 4/5] drivers/net: not use private eth dev data Jianfeng Tan
2018-04-24 5:51 ` [dpdk-dev] [PATCH v5 5/5] drivers/net: share vdev data to secondary process Jianfeng Tan
2018-04-24 10:32 ` [dpdk-dev] [PATCH v5 0/5] allow procinfo and pdump on eth vdev Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM4PR0501MB26578466FF367876668DCE40D2D90@AM4PR0501MB2657.eurprd05.prod.outlook.com \
--to=matan@mellanox.com \
--cc=anatoly.burakov@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=jianfeng.tan@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).