[dpdk-dev,PATCHv4,2/9] null: fix segfault when null_pmd added to bonding

Message ID 1436981189-3320-3-git-send-email-tomaszx.kulasek@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Tomasz Kulasek July 15, 2015, 5:26 p.m. UTC
  When device is added to the bonding, the link status callback is added to
the slave's eth_dev->link_intr_cbs list. This list is not initialized for
null pmd and adding it to the bonding segfaults application.

This patch allocates and sets up required structures.

Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 drivers/net/null/rte_eth_null.c |   11 +++++++++++
 1 file changed, 11 insertions(+)
  

Comments

Tetsuya Mukawa Sept. 29, 2015, 2:24 a.m. UTC | #1
On 2015/07/16 2:26, Tomasz Kulasek wrote:
> When device is added to the bonding, the link status callback is added to
> the slave's eth_dev->link_intr_cbs list. This list is not initialized for
> null pmd and adding it to the bonding segfaults application.
>
> This patch allocates and sets up required structures.
>
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> ---
>  drivers/net/null/rte_eth_null.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
> index e244595..a8b3191 100644
> --- a/drivers/net/null/rte_eth_null.c
> +++ b/drivers/net/null/rte_eth_null.c
> @@ -386,6 +386,7 @@ eth_dev_null_create(const char *name,
>  	const unsigned nb_rx_queues = 1;
>  	const unsigned nb_tx_queues = 1;
>  	struct rte_eth_dev_data *data = NULL;
> +	struct eth_driver *eth_drv = NULL;

Hi Tomasz,

Thanks for extending null pmd features.
Is it possible to use rte_null_pmd here?
Could you please check ring pmd? It may also uses rte_ring_pmd for link
status callback.

Tetsuya
  
Tomasz Kulasek Sept. 29, 2015, 9:39 a.m. UTC | #2
Hi Tetsuya,

> Thanks for extending null pmd features.
> Is it possible to use rte_null_pmd here?
> Could you please check ring pmd? It may also uses rte_ring_pmd for link
> status callback.
> 
> Tetsuya

My first attempt was to use ring pmd, and there's no such an issue with it. It works pretty well in bonding.

Tomasz.
  
Tetsuya Mukawa Sept. 29, 2015, 10:32 a.m. UTC | #3
On 2015/09/29 18:39, Kulasek, TomaszX wrote:
> Hi Tetsuya,
>
>> Thanks for extending null pmd features.
>> Is it possible to use rte_null_pmd here?
>> Could you please check ring pmd? It may also uses rte_ring_pmd for link
>> status callback.
>>
>> Tetsuya
> My first attempt was to use ring pmd, and there's no such an issue with it. It works pretty well in bonding.
>
> Tomasz.

HI Tomasz,


Sorry, my English is wrong.
'rte_null_pmd' is defined like below.

static struct eth_driver rte_null_pmd = {
        .pci_drv = {
                .name = "rte_null_pmd",
                .drv_flags = RTE_PCI_DRV_DETACHABLE,
        },
};

I guess you may be able to use 'rte_null_pmd' instead of allocating one
more eth_driver structure like below.

    struct eth_driver *eth_drv = NULL;
    eth_drv = rte_zmalloc_socket(name, sizeof(*eth_drv), 0, numa_node);

Is it possible to use 'rte_null_pmd'?

Tetsuya
  
Tomasz Kulasek Sept. 29, 2015, 11:29 a.m. UTC | #4
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, September 29, 2015 12:33
> To: Kulasek, TomaszX
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCHv4 2/9] null: fix segfault when null_pmd
> added to bonding
> 
> On 2015/09/29 18:39, Kulasek, TomaszX wrote:
> > Hi Tetsuya,
> >
> >> Thanks for extending null pmd features.
> >> Is it possible to use rte_null_pmd here?
> >> Could you please check ring pmd? It may also uses rte_ring_pmd for
> >> link status callback.
> >>
> >> Tetsuya
> > My first attempt was to use ring pmd, and there's no such an issue
> with it. It works pretty well in bonding.
> >
> > Tomasz.
> 
> HI Tomasz,
> 
> 
> Sorry, my English is wrong.
> 'rte_null_pmd' is defined like below.
> 
> static struct eth_driver rte_null_pmd = {
>         .pci_drv = {
>                 .name = "rte_null_pmd",
>                 .drv_flags = RTE_PCI_DRV_DETACHABLE,
>         },
> };
> 
> I guess you may be able to use 'rte_null_pmd' instead of allocating one
> more eth_driver structure like below.
> 
>     struct eth_driver *eth_drv = NULL;
>     eth_drv = rte_zmalloc_socket(name, sizeof(*eth_drv), 0, numa_node);
> 
> Is it possible to use 'rte_null_pmd'?
> 
> Tetsuya

Yes, you're right. This malloc can be removed.

Tomasz
  

Patch

diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index e244595..a8b3191 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -386,6 +386,7 @@  eth_dev_null_create(const char *name,
 	const unsigned nb_rx_queues = 1;
 	const unsigned nb_tx_queues = 1;
 	struct rte_eth_dev_data *data = NULL;
+	struct eth_driver *eth_drv = NULL;
 	struct rte_pci_device *pci_dev = NULL;
 	struct pmd_internals *internals = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
@@ -416,6 +417,10 @@  eth_dev_null_create(const char *name,
 	if (eth_dev == NULL)
 		goto error;
 
+	eth_drv = rte_zmalloc_socket(name, sizeof(*eth_drv), 0, numa_node);
+	if (!eth_drv)
+		goto error;
+
 	/* now put it all together
 	 * - store queue data in internals,
 	 * - store numa_node info in pci_driver
@@ -431,7 +436,10 @@  eth_dev_null_create(const char *name,
 	internals->packet_copy = packet_copy;
 	internals->numa_node = numa_node;
 
+	eth_drv->pci_drv.name = drivername;
+
 	pci_dev->numa_node = numa_node;
+	pci_dev->driver = &eth_drv->pci_drv;
 
 	data->dev_private = internals;
 	data->port_id = eth_dev->data->port_id;
@@ -445,6 +453,7 @@  eth_dev_null_create(const char *name,
 	eth_dev->dev_ops = &ops;
 	eth_dev->pci_dev = pci_dev;
 	eth_dev->driver = &rte_null_pmd;
+	TAILQ_INIT(&eth_dev->link_intr_cbs);
 
 	/* finally assign rx and tx ops */
 	if (packet_copy) {
@@ -461,6 +470,8 @@  error:
 	rte_free(data);
 	rte_free(pci_dev);
 	rte_free(internals);
+	rte_free(eth_dev);
+	rte_free(eth_drv);
 
 	return -1;
 }