[dpdk-dev] [RFC] af_packet: support port hotplug

[dpdk-dev] [RFC] af_packet: support port hotplug

[John W. Linville]

This patch adds finalization code to free resources allocated by
the PMD.  This is based on earlier patches for other PMDs by Tetsuya
Mukawa <mukawa@igel.co.jp>, with corrections related to data->name.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56 ++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf7f819..57998ab19c96 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
 	.stats_reset = eth_stats_reset,
 };
 
+static struct eth_driver rte_af_packet_pmd = {
+	.pci_drv = {
+		.name = "rte_af_packet_pmd",
+		.drv_flags = RTE_PCI_DRV_DETACHABLE,
+	},
+};
+
 /*
  * Opens an AF_PACKET socket
  */
@@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
 	if (*eth_dev == NULL)
 		goto error;
 
+	/* check length of device name */
+	if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
+		goto error;
+
 	/*
 	 * now put it all together
 	 * - store queue data in internals,
@@ -669,12 +680,14 @@ rte_pmd_init_internals(const char *name,
 	data->nb_tx_queues = (uint16_t)nb_queues;
 	data->dev_link = pmd_link;
 	data->mac_addrs = &(*internals)->eth_addr;
+	strncpy(data->name, (*eth_dev)->data->name, strlen((*eth_dev)->data->name));
 
 	pci_dev->numa_node = numa_node;
 
 	(*eth_dev)->data = data;
 	(*eth_dev)->dev_ops = &ops;
 	(*eth_dev)->pci_dev = pci_dev;
+	(*eth_dev)->driver = &rte_af_packet_pmd;
 
 	return 0;
 
@@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 	return 0;
 }
 
+static int
+rte_pmd_af_packet_devuninit(const char *name)
+{
+	struct rte_eth_dev *eth_dev = NULL;
+	struct pmd_internals *internals;
+	struct tpacket_req req;
+
+	unsigned q;
+
+	RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
+			rte_socket_id());
+
+	if (name == NULL)
+		return -1;
+
+	/* retrieve ethdev entry */
+	eth_dev = rte_eth_dev_allocated(name);
+	if (eth_dev == NULL)
+		return -1;
+
+	internals = eth_dev->data->dev_private;
+	req = internals->req;
+
+	for (q = 0; q < internals->nb_queues; q++) {
+		munmap(internals->rx_queue[q].map,
+		       2 * req.tp_block_size * req.tp_block_nr);
+		if (internals->rx_queue[q].rd)
+			rte_free(internals->rx_queue[q].rd);
+		if (internals->tx_queue[q].rd)
+			rte_free(internals->tx_queue[q].rd);
+	}
+
+	rte_free(internals);
+	rte_free(eth_dev->data);
+	rte_free(eth_dev->pci_dev);
+
+	rte_eth_dev_release_port(eth_dev);
+
+
+	return 0;
+}
+
 static struct rte_driver pmd_af_packet_drv = {
 	.name = "eth_af_packet",
 	.type = PMD_VDEV,
 	.init = rte_pmd_af_packet_devinit,
+	.uninit = rte_pmd_af_packet_devuninit,
 };
 
 PMD_REGISTER_DRIVER(pmd_af_packet_drv);
-- 
2.1.0

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Tetsuya Mukawa]

Hi John.

On 2015/03/11 3:36, John W. Linville wrote:
> This patch adds finalization code to free resources allocated by
> the PMD.  This is based on earlier patches for other PMDs by Tetsuya
> Mukawa <mukawa@igel.co.jp>, with corrections related to data->name.
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56 ++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 80e9bdf7f819..57998ab19c96 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
>  	.stats_reset = eth_stats_reset,
>  };
>  
> +static struct eth_driver rte_af_packet_pmd = {
> +	.pci_drv = {
> +		.name = "rte_af_packet_pmd",
> +		.drv_flags = RTE_PCI_DRV_DETACHABLE,
> +	},
> +};
> +
>  /*
>   * Opens an AF_PACKET socket
>   */
> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
>  	if (*eth_dev == NULL)
>  		goto error;
>  
> +	/* check length of device name */
> +	if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
> +		goto error;
> +
>  	/*
>  	 * now put it all together
>  	 * - store queue data in internals,
> @@ -669,12 +680,14 @@ rte_pmd_init_internals(const char *name,
>  	data->nb_tx_queues = (uint16_t)nb_queues;
>  	data->dev_link = pmd_link;
>  	data->mac_addrs = &(*internals)->eth_addr;
> +	strncpy(data->name, (*eth_dev)->data->name, strlen((*eth_dev)->data->name));

Above line is over 80 characters.
(I am not sure we need to keep it, but may be good.)

>  
>  	pci_dev->numa_node = numa_node;
>  
>  	(*eth_dev)->data = data;
>  	(*eth_dev)->dev_ops = &ops;
>  	(*eth_dev)->pci_dev = pci_dev;
> +	(*eth_dev)->driver = &rte_af_packet_pmd;
>  
>  	return 0;
>  
> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>  	return 0;
>  }
>  
> +static int
> +rte_pmd_af_packet_devuninit(const char *name)
> +{
> +	struct rte_eth_dev *eth_dev = NULL;
> +	struct pmd_internals *internals;
> +	struct tpacket_req req;
> +
> +	unsigned q;
> +
> +	RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
> +			rte_socket_id());
> +
> +	if (name == NULL)
> +		return -1;
> +
> +	/* retrieve ethdev entry */
> +	eth_dev = rte_eth_dev_allocated(name);
> +	if (eth_dev == NULL)
> +		return -1;
> +
> +	internals = eth_dev->data->dev_private;
> +	req = internals->req;
> +
> +	for (q = 0; q < internals->nb_queues; q++) {
> +		munmap(internals->rx_queue[q].map,
> +		       2 * req.tp_block_size * req.tp_block_nr);
> +		if (internals->rx_queue[q].rd)
> +			rte_free(internals->rx_queue[q].rd);
> +		if (internals->tx_queue[q].rd)
> +			rte_free(internals->tx_queue[q].rd);
> +	}
> +
> +	rte_free(internals);
> +	rte_free(eth_dev->data);
> +	rte_free(eth_dev->pci_dev);
> +
> +	rte_eth_dev_release_port(eth_dev);
> +
> +
> +	return 0;
> +}
> +
>  static struct rte_driver pmd_af_packet_drv = {
>  	.name = "eth_af_packet",
>  	.type = PMD_VDEV,
>  	.init = rte_pmd_af_packet_devinit,
> +	.uninit = rte_pmd_af_packet_devuninit,
>  };
>  
>  PMD_REGISTER_DRIVER(pmd_af_packet_drv);

Except above, I guess it's good.
I've checked followings.

- Code review
- No missing symbol
- No compile error with icc
- No compile error with gcc-4.6
- No compile error with gcc-4.7
- No compile error with gcc-4.8
- No compile error with gcc-4.9
- checkpatch.pl

Thanks,
Tetsuya

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Iremonger, Bernard]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of John W. Linville
> Sent: Tuesday, March 10, 2015 6:36 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug
> 
> This patch adds finalization code to free resources allocated by the PMD.  This is based on earlier
> patches for other PMDs by Tetsuya Mukawa <mukawa@igel.co.jp>, with corrections related to data-
> >name.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56 ++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 80e9bdf7f819..57998ab19c96 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
>  	.stats_reset = eth_stats_reset,
>  };
> 
> +static struct eth_driver rte_af_packet_pmd = {
> +	.pci_drv = {
> +		.name = "rte_af_packet_pmd",
> +		.drv_flags = RTE_PCI_DRV_DETACHABLE,
> +	},
> +};
> +
>  /*
>   * Opens an AF_PACKET socket
>   */
> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
>  	if (*eth_dev == NULL)
>  		goto error;
> 
> +	/* check length of device name */
> +	if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
> +		goto error;
> +
>  	/*
>  	 * now put it all together
>  	 * - store queue data in internals,
> @@ -669,12 +680,14 @@ rte_pmd_init_internals(const char *name,
>  	data->nb_tx_queues = (uint16_t)nb_queues;
>  	data->dev_link = pmd_link;
>  	data->mac_addrs = &(*internals)->eth_addr;
> +	strncpy(data->name, (*eth_dev)->data->name,
> +strlen((*eth_dev)->data->name));
> 
>  	pci_dev->numa_node = numa_node;
> 
>  	(*eth_dev)->data = data;
>  	(*eth_dev)->dev_ops = &ops;
>  	(*eth_dev)->pci_dev = pci_dev;
> +	(*eth_dev)->driver = &rte_af_packet_pmd;
> 
>  	return 0;
> 
> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>  	return 0;
>  }
> 
> +static int
> +rte_pmd_af_packet_devuninit(const char *name) {
> +	struct rte_eth_dev *eth_dev = NULL;
> +	struct pmd_internals *internals;
> +	struct tpacket_req req;
> +
> +	unsigned q;
> +
> +	RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
> +			rte_socket_id());
> +
> +	if (name == NULL)
> +		return -1;

Hi  Tetsuya, John,

Before detaching a port, the port must be stopped and closed.
The stop and close are only allowed for RTE_PROC_PRIMARY.
Should there be a check for process_type here?

if (rte_eal_process_type() != RTE_PROC_PRIMARY)
	return -EPERM;

Regards,

Bernard

> +
> +	/* retrieve ethdev entry */
> +	eth_dev = rte_eth_dev_allocated(name);
> +	if (eth_dev == NULL)
> +		return -1;
> +
> +	internals = eth_dev->data->dev_private;
> +	req = internals->req;
> +
> +	for (q = 0; q < internals->nb_queues; q++) {
> +		munmap(internals->rx_queue[q].map,
> +		       2 * req.tp_block_size * req.tp_block_nr);
> +		if (internals->rx_queue[q].rd)
> +			rte_free(internals->rx_queue[q].rd);
> +		if (internals->tx_queue[q].rd)
> +			rte_free(internals->tx_queue[q].rd);
> +	}
> +
> +	rte_free(internals);
> +	rte_free(eth_dev->data);
> +	rte_free(eth_dev->pci_dev);
> +
> +	rte_eth_dev_release_port(eth_dev);
> +
> +
> +	return 0;
> +}
> +
>  static struct rte_driver pmd_af_packet_drv = {
>  	.name = "eth_af_packet",
>  	.type = PMD_VDEV,
>  	.init = rte_pmd_af_packet_devinit,
> +	.uninit = rte_pmd_af_packet_devuninit,
>  };
> 
>  PMD_REGISTER_DRIVER(pmd_af_packet_drv);
> --
> 2.1.0

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Tetsuya Mukawa]

2015-03-13 2:05 GMT+09:00 Iremonger, Bernard <bernard.iremonger@intel.com>:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of John W. Linville
>> Sent: Tuesday, March 10, 2015 6:36 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug
>>
>> This patch adds finalization code to free resources allocated by the PMD.  This is based on earlier
>> patches for other PMDs by Tetsuya Mukawa <mukawa@igel.co.jp>, with corrections related to data-
>> >name.
>>
>> Signed-off-by: John W. Linville <linville@tuxdriver.com>
>> Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56 ++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> index 80e9bdf7f819..57998ab19c96 100644
>> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
>>       .stats_reset = eth_stats_reset,
>>  };
>>
>> +static struct eth_driver rte_af_packet_pmd = {
>> +     .pci_drv = {
>> +             .name = "rte_af_packet_pmd",
>> +             .drv_flags = RTE_PCI_DRV_DETACHABLE,
>> +     },
>> +};
>> +
>>  /*
>>   * Opens an AF_PACKET socket
>>   */
>> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
>>       if (*eth_dev == NULL)
>>               goto error;
>>
>> +     /* check length of device name */
>> +     if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
>> +             goto error;
>> +
>>       /*
>>        * now put it all together
>>        * - store queue data in internals,
>> @@ -669,12 +680,14 @@ rte_pmd_init_internals(const char *name,
>>       data->nb_tx_queues = (uint16_t)nb_queues;
>>       data->dev_link = pmd_link;
>>       data->mac_addrs = &(*internals)->eth_addr;
>> +     strncpy(data->name, (*eth_dev)->data->name,
>> +strlen((*eth_dev)->data->name));
>>
>>       pci_dev->numa_node = numa_node;
>>
>>       (*eth_dev)->data = data;
>>       (*eth_dev)->dev_ops = &ops;
>>       (*eth_dev)->pci_dev = pci_dev;
>> +     (*eth_dev)->driver = &rte_af_packet_pmd;
>>
>>       return 0;
>>
>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>>       return 0;
>>  }
>>
>> +static int
>> +rte_pmd_af_packet_devuninit(const char *name) {
>> +     struct rte_eth_dev *eth_dev = NULL;
>> +     struct pmd_internals *internals;
>> +     struct tpacket_req req;
>> +
>> +     unsigned q;
>> +
>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
>> +                     rte_socket_id());
>> +
>> +     if (name == NULL)
>> +             return -1;
>
> Hi  Tetsuya, John,
>
> Before detaching a port, the port must be stopped and closed.
> The stop and close are only allowed for RTE_PROC_PRIMARY.
> Should there be a check for process_type here?
>
> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>         return -EPERM;
>
> Regards,
>
> Bernard
>

Hi Bernard,

I agree with stop() and close() are only called by primary process,
but it may not need to add like above.
Could you please check rte_ethdev.c?

- struct rte_eth_dev_data *rte_eth_dev_data;
This array is shared between processes.
So we need to initialize of finalize carefully like you said.

- struct rte_eth_dev rte_eth_devices[]
This array is per process.
And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.

All PMDs for physical NIC allocates like above when PMDs are initialized.
(Even when a process is secondary, initialization function of PMDs
will be called)
But virtual device PMDs allocate rte_eth_dev_data and overwrite 'data'
variable of rte_eth_devices while initialization.

As a result, primary and secondary process has their own
'rte_eth_dev_data' for a virtual device.
So I guess all processes need to free it not to leak memory.

Thanks,
Tetsuya

>> +
>> +     /* retrieve ethdev entry */
>> +     eth_dev = rte_eth_dev_allocated(name);
>> +     if (eth_dev == NULL)
>> +             return -1;
>> +
>> +     internals = eth_dev->data->dev_private;
>> +     req = internals->req;
>> +
>> +     for (q = 0; q < internals->nb_queues; q++) {
>> +             munmap(internals->rx_queue[q].map,
>> +                    2 * req.tp_block_size * req.tp_block_nr);
>> +             if (internals->rx_queue[q].rd)
>> +                     rte_free(internals->rx_queue[q].rd);
>> +             if (internals->tx_queue[q].rd)
>> +                     rte_free(internals->tx_queue[q].rd);
>> +     }
>> +
>> +     rte_free(internals);
>> +     rte_free(eth_dev->data);
>> +     rte_free(eth_dev->pci_dev);
>> +
>> +     rte_eth_dev_release_port(eth_dev);
>> +
>> +
>> +     return 0;
>> +}
>> +
>>  static struct rte_driver pmd_af_packet_drv = {
>>       .name = "eth_af_packet",
>>       .type = PMD_VDEV,
>>       .init = rte_pmd_af_packet_devinit,
>> +     .uninit = rte_pmd_af_packet_devuninit,
>>  };
>>
>>  PMD_REGISTER_DRIVER(pmd_af_packet_drv);
>> --
>> 2.1.0
>

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Iremonger, Bernard]

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Friday, March 13, 2015 12:11 AM
> To: Iremonger, Bernard
> Cc: John W. Linville; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
> 
> 2015-03-13 2:05 GMT+09:00 Iremonger, Bernard <bernard.iremonger@intel.com>:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of John W. Linville
> >> Sent: Tuesday, March 10, 2015 6:36 PM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug
> >>
> >> This patch adds finalization code to free resources allocated by the
> >> PMD.  This is based on earlier patches for other PMDs by Tetsuya
> >> Mukawa <mukawa@igel.co.jp>, with corrections related to data-
> >> >name.
> >>
> >> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> >> Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
> >> ---
> >>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56
> >> ++++++++++++++++++++++++++++
> >>  1 file changed, 56 insertions(+)
> >>
> >> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> >> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> >> index 80e9bdf7f819..57998ab19c96 100644
> >> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> >> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> >> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
> >>       .stats_reset = eth_stats_reset,  };
> >>
> >> +static struct eth_driver rte_af_packet_pmd = {
> >> +     .pci_drv = {
> >> +             .name = "rte_af_packet_pmd",
> >> +             .drv_flags = RTE_PCI_DRV_DETACHABLE,
> >> +     },
> >> +};
> >> +
> >>  /*
> >>   * Opens an AF_PACKET socket
> >>   */
> >> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
> >>       if (*eth_dev == NULL)
> >>               goto error;
> >>
> >> +     /* check length of device name */
> >> +     if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
> >> +             goto error;
> >> +
> >>       /*
> >>        * now put it all together
> >>        * - store queue data in internals, @@ -669,12 +680,14 @@
> >> rte_pmd_init_internals(const char *name,
> >>       data->nb_tx_queues = (uint16_t)nb_queues;
> >>       data->dev_link = pmd_link;
> >>       data->mac_addrs = &(*internals)->eth_addr;
> >> +     strncpy(data->name, (*eth_dev)->data->name,
> >> +strlen((*eth_dev)->data->name));
> >>
> >>       pci_dev->numa_node = numa_node;
> >>
> >>       (*eth_dev)->data = data;
> >>       (*eth_dev)->dev_ops = &ops;
> >>       (*eth_dev)->pci_dev = pci_dev;
> >> +     (*eth_dev)->driver = &rte_af_packet_pmd;
> >>
> >>       return 0;
> >>
> >> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
> >>       return 0;
> >>  }
> >>
> >> +static int
> >> +rte_pmd_af_packet_devuninit(const char *name) {
> >> +     struct rte_eth_dev *eth_dev = NULL;
> >> +     struct pmd_internals *internals;
> >> +     struct tpacket_req req;
> >> +
> >> +     unsigned q;
> >> +
> >> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
> >> +                     rte_socket_id());
> >> +
> >> +     if (name == NULL)
> >> +             return -1;
> >
> > Hi  Tetsuya, John,
> >
> > Before detaching a port, the port must be stopped and closed.
> > The stop and close are only allowed for RTE_PROC_PRIMARY.
> > Should there be a check for process_type here?
> >
> > if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >         return -EPERM;
> >
> > Regards,
> >
> > Bernard
> >
> 
> Hi Bernard,
> 
> I agree with stop() and close() are only called by primary process, but it may not need to add like
> above.
> Could you please check rte_ethdev.c?
> 
> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between processes.
> So we need to initialize of finalize carefully like you said.
> 
> - struct rte_eth_dev rte_eth_devices[]
> This array is per process.
> And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.
> 
> All PMDs for physical NIC allocates like above when PMDs are initialized.
> (Even when a process is secondary, initialization function of PMDs will be called) But virtual device
> PMDs allocate rte_eth_dev_data and overwrite 'data'
> variable of rte_eth_devices while initialization.
> 
> As a result, primary and secondary process has their own 'rte_eth_dev_data' for a virtual device.
> So I guess all processes need to free it not to leak memory.
> 
> Thanks,
> Tetsuya
> 
Hi Tetsuya,

In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro  PROC_PRIMARY_OR_RET().
So for secondary processes  both functions return without doing anything. 
Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?

For the Physical/Virtual  Functions of the NIC  a lot of the finalization is done in the  dev->dev_ops->dev_stop() and
dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is called, this should probably do nothing for secondary processes  as the dev_stop() and dev_close() functions will not have been executed.

For the Physical/Virtual  Functions of the NIC  the dev_init() is called for both primary and secondary processes, however a subset of the function only is executed for secondary processes.

Regards,

Bernard.
 

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Tetsuya Mukawa]

On 2015/03/13 19:14, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Friday, March 13, 2015 12:11 AM
>> To: Iremonger, Bernard
>> Cc: John W. Linville; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
>>
>> 2015-03-13 2:05 GMT+09:00 Iremonger, Bernard <bernard.iremonger@intel.com>:
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of John W. Linville
>>>> Sent: Tuesday, March 10, 2015 6:36 PM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug
>>>>
>>>> This patch adds finalization code to free resources allocated by the
>>>> PMD.  This is based on earlier patches for other PMDs by Tetsuya
>>>> Mukawa <mukawa@igel.co.jp>, with corrections related to data-
>>>>> name.
>>>> Signed-off-by: John W. Linville <linville@tuxdriver.com>
>>>> Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
>>>> ---
>>>>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56
>>>> ++++++++++++++++++++++++++++
>>>>  1 file changed, 56 insertions(+)
>>>>
>>>> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>> index 80e9bdf7f819..57998ab19c96 100644
>>>> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
>>>>       .stats_reset = eth_stats_reset,  };
>>>>
>>>> +static struct eth_driver rte_af_packet_pmd = {
>>>> +     .pci_drv = {
>>>> +             .name = "rte_af_packet_pmd",
>>>> +             .drv_flags = RTE_PCI_DRV_DETACHABLE,
>>>> +     },
>>>> +};
>>>> +
>>>>  /*
>>>>   * Opens an AF_PACKET socket
>>>>   */
>>>> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
>>>>       if (*eth_dev == NULL)
>>>>               goto error;
>>>>
>>>> +     /* check length of device name */
>>>> +     if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
>>>> +             goto error;
>>>> +
>>>>       /*
>>>>        * now put it all together
>>>>        * - store queue data in internals, @@ -669,12 +680,14 @@
>>>> rte_pmd_init_internals(const char *name,
>>>>       data->nb_tx_queues = (uint16_t)nb_queues;
>>>>       data->dev_link = pmd_link;
>>>>       data->mac_addrs = &(*internals)->eth_addr;
>>>> +     strncpy(data->name, (*eth_dev)->data->name,
>>>> +strlen((*eth_dev)->data->name));
>>>>
>>>>       pci_dev->numa_node = numa_node;
>>>>
>>>>       (*eth_dev)->data = data;
>>>>       (*eth_dev)->dev_ops = &ops;
>>>>       (*eth_dev)->pci_dev = pci_dev;
>>>> +     (*eth_dev)->driver = &rte_af_packet_pmd;
>>>>
>>>>       return 0;
>>>>
>>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>>>>       return 0;
>>>>  }
>>>>
>>>> +static int
>>>> +rte_pmd_af_packet_devuninit(const char *name) {
>>>> +     struct rte_eth_dev *eth_dev = NULL;
>>>> +     struct pmd_internals *internals;
>>>> +     struct tpacket_req req;
>>>> +
>>>> +     unsigned q;
>>>> +
>>>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
>>>> +                     rte_socket_id());
>>>> +
>>>> +     if (name == NULL)
>>>> +             return -1;
>>> Hi  Tetsuya, John,
>>>
>>> Before detaching a port, the port must be stopped and closed.
>>> The stop and close are only allowed for RTE_PROC_PRIMARY.
>>> Should there be a check for process_type here?
>>>
>>> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>         return -EPERM;
>>>
>>> Regards,
>>>
>>> Bernard
>>>
>> Hi Bernard,
>>
>> I agree with stop() and close() are only called by primary process, but it may not need to add like
>> above.
>> Could you please check rte_ethdev.c?
>>
>> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between processes.
>> So we need to initialize of finalize carefully like you said.
>>
>> - struct rte_eth_dev rte_eth_devices[]
>> This array is per process.
>> And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.
>>
>> All PMDs for physical NIC allocates like above when PMDs are initialized.
>> (Even when a process is secondary, initialization function of PMDs will be called) But virtual device
>> PMDs allocate rte_eth_dev_data and overwrite 'data'
>> variable of rte_eth_devices while initialization.
>>
>> As a result, primary and secondary process has their own 'rte_eth_dev_data' for a virtual device.
>> So I guess all processes need to free it not to leak memory.
>>
>> Thanks,
>> Tetsuya
>>
> Hi Tetsuya,
>
> In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro  PROC_PRIMARY_OR_RET().
> So for secondary processes  both functions return without doing anything. 
> Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?
>
> For the Physical/Virtual  Functions of the NIC  a lot of the finalization is done in the  dev->dev_ops->dev_stop() and
> dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is called, this should probably do nothing for secondary processes  as the dev_stop() and dev_close() functions will not have been executed.

Hi Bernard,

Sorry for my English.
I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs.
Not a PMDs for virtual functions on NIC.

For PMDs like a pcap and af_packet PMDs, all data structures are
allocated per processes.
(Actually I guess nothing is shared between primary and secondary
processes, because rte_eth_dev_data is overwritten by each processes.)
So we need to free per process data when detach() is called.

> For the Physical/Virtual  Functions of the NIC  the dev_init() is called for both primary and secondary processes, however a subset of the function only is executed for secondary processes.

Because of above, we probably not be able to add PROC_PRIMARY_OR_RET()
to rte_eth_dev_detach().
But I agree we should not call rte_eth_dev_detach() for secondary
process, if PMDs are like e1000 or ixgbe PMD.

To work like above, how about changing drv_flags dynamically in close()
callback?
For example, when primary process calls rte_eth_dev_close(), a callback
of PMD will be called.
(In the case of e1000 PMD, eth_em_close() is the callback.)

At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the
callback.
It means if primary process hasn't called close() yet,
rte_eth_dev_detach() will do nothing and return error.
How about doing like above?

Regards,
Tetsuya

> Regards,
>
> Bernard.
>  
>
>
>
>
>
>

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Tetsuya Mukawa]

On 2015/03/16 17:38, Tetsuya Mukawa wrote:
> On 2015/03/13 19:14, Iremonger, Bernard wrote:
>>> -----Original Message-----
>>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>>> Sent: Friday, March 13, 2015 12:11 AM
>>> To: Iremonger, Bernard
>>> Cc: John W. Linville; dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
>>>
>>> 2015-03-13 2:05 GMT+09:00 Iremonger, Bernard <bernard.iremonger@intel.com>:
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of John W. Linville
>>>>> Sent: Tuesday, March 10, 2015 6:36 PM
>>>>> To: dev@dpdk.org
>>>>> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug
>>>>>
>>>>> This patch adds finalization code to free resources allocated by the
>>>>> PMD.  This is based on earlier patches for other PMDs by Tetsuya
>>>>> Mukawa <mukawa@igel.co.jp>, with corrections related to data-
>>>>>> name.
>>>>> Signed-off-by: John W. Linville <linville@tuxdriver.com>
>>>>> Cc: Tetsuya Mukawa <mukawa@igel.co.jp>
>>>>> ---
>>>>>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56
>>>>> ++++++++++++++++++++++++++++
>>>>>  1 file changed, 56 insertions(+)
>>>>>
>>>>> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>>> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>>> index 80e9bdf7f819..57998ab19c96 100644
>>>>> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>>> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
>>>>> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops = {
>>>>>       .stats_reset = eth_stats_reset,  };
>>>>>
>>>>> +static struct eth_driver rte_af_packet_pmd = {
>>>>> +     .pci_drv = {
>>>>> +             .name = "rte_af_packet_pmd",
>>>>> +             .drv_flags = RTE_PCI_DRV_DETACHABLE,
>>>>> +     },
>>>>> +};
>>>>> +
>>>>>  /*
>>>>>   * Opens an AF_PACKET socket
>>>>>   */
>>>>> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name,
>>>>>       if (*eth_dev == NULL)
>>>>>               goto error;
>>>>>
>>>>> +     /* check length of device name */
>>>>> +     if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))
>>>>> +             goto error;
>>>>> +
>>>>>       /*
>>>>>        * now put it all together
>>>>>        * - store queue data in internals, @@ -669,12 +680,14 @@
>>>>> rte_pmd_init_internals(const char *name,
>>>>>       data->nb_tx_queues = (uint16_t)nb_queues;
>>>>>       data->dev_link = pmd_link;
>>>>>       data->mac_addrs = &(*internals)->eth_addr;
>>>>> +     strncpy(data->name, (*eth_dev)->data->name,
>>>>> +strlen((*eth_dev)->data->name));
>>>>>
>>>>>       pci_dev->numa_node = numa_node;
>>>>>
>>>>>       (*eth_dev)->data = data;
>>>>>       (*eth_dev)->dev_ops = &ops;
>>>>>       (*eth_dev)->pci_dev = pci_dev;
>>>>> +     (*eth_dev)->driver = &rte_af_packet_pmd;
>>>>>
>>>>>       return 0;
>>>>>
>>>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> +static int
>>>>> +rte_pmd_af_packet_devuninit(const char *name) {
>>>>> +     struct rte_eth_dev *eth_dev = NULL;
>>>>> +     struct pmd_internals *internals;
>>>>> +     struct tpacket_req req;
>>>>> +
>>>>> +     unsigned q;
>>>>> +
>>>>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
>>>>> +                     rte_socket_id());
>>>>> +
>>>>> +     if (name == NULL)
>>>>> +             return -1;
>>>> Hi  Tetsuya, John,
>>>>
>>>> Before detaching a port, the port must be stopped and closed.
>>>> The stop and close are only allowed for RTE_PROC_PRIMARY.
>>>> Should there be a check for process_type here?
>>>>
>>>> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>         return -EPERM;
>>>>
>>>> Regards,
>>>>
>>>> Bernard
>>>>
>>> Hi Bernard,
>>>
>>> I agree with stop() and close() are only called by primary process, but it may not need to add like
>>> above.
>>> Could you please check rte_ethdev.c?
>>>
>>> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between processes.
>>> So we need to initialize of finalize carefully like you said.
>>>
>>> - struct rte_eth_dev rte_eth_devices[]
>>> This array is per process.
>>> And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.
>>>
>>> All PMDs for physical NIC allocates like above when PMDs are initialized.
>>> (Even when a process is secondary, initialization function of PMDs will be called) But virtual device
>>> PMDs allocate rte_eth_dev_data and overwrite 'data'
>>> variable of rte_eth_devices while initialization.
>>>
>>> As a result, primary and secondary process has their own 'rte_eth_dev_data' for a virtual device.
>>> So I guess all processes need to free it not to leak memory.
>>>
>>> Thanks,
>>> Tetsuya
>>>
>> Hi Tetsuya,
>>
>> In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro  PROC_PRIMARY_OR_RET().
>> So for secondary processes  both functions return without doing anything. 
>> Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?
>>
>> For the Physical/Virtual  Functions of the NIC  a lot of the finalization is done in the  dev->dev_ops->dev_stop() and
>> dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is called, this should probably do nothing for secondary processes  as the dev_stop() and dev_close() functions will not have been executed.
> Hi Bernard,
>
> Sorry for my English.
> I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs.
> Not a PMDs for virtual functions on NIC.
>
> For PMDs like a pcap and af_packet PMDs, all data structures are
> allocated per processes.
> (Actually I guess nothing is shared between primary and secondary
> processes, because rte_eth_dev_data is overwritten by each processes.)
> So we need to free per process data when detach() is called.
>
>> For the Physical/Virtual  Functions of the NIC  the dev_init() is called for both primary and secondary processes, however a subset of the function only is executed for secondary processes.
> Because of above, we probably not be able to add PROC_PRIMARY_OR_RET()
> to rte_eth_dev_detach().
> But I agree we should not call rte_eth_dev_detach() for secondary
> process, if PMDs are like e1000 or ixgbe PMD.

Correction:
We should not process rte_eth_dev_detach() for secondary process, if
PMDs are like e1000 or ixgbe PMD and if primary process hasn't called
stop() and close() yet.

Tetsuya

>
> To work like above, how about changing drv_flags dynamically in close()
> callback?
> For example, when primary process calls rte_eth_dev_close(), a callback
> of PMD will be called.
> (In the case of e1000 PMD, eth_em_close() is the callback.)
>
> At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the
> callback.
> It means if primary process hasn't called close() yet,
> rte_eth_dev_detach() will do nothing and return error.
> How about doing like above?
>
> Regards,
> Tetsuya
>
>> Regards,
>>
>> Bernard.
>>  
>>
>>
>>
>>
>>
>>
>

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Iremonger, Bernard]

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Monday, March 16, 2015 8:57 AM
> To: Iremonger, Bernard
> Cc: John W. Linville; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
> 
> >>>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
> >>>>>       return 0;
> >>>>>  }
> >>>>>
> >>>>> +static int
> >>>>> +rte_pmd_af_packet_devuninit(const char *name) {
> >>>>> +     struct rte_eth_dev *eth_dev = NULL;
> >>>>> +     struct pmd_internals *internals;
> >>>>> +     struct tpacket_req req;
> >>>>> +
> >>>>> +     unsigned q;
> >>>>> +
> >>>>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
> >>>>> +                     rte_socket_id());
> >>>>> +
> >>>>> +     if (name == NULL)
> >>>>> +             return -1;
> >>>> Hi  Tetsuya, John,
> >>>>
> >>>> Before detaching a port, the port must be stopped and closed.
> >>>> The stop and close are only allowed for RTE_PROC_PRIMARY.
> >>>> Should there be a check for process_type here?
> >>>>
> >>>> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>>>         return -EPERM;
> >>>>
> >>>> Regards,
> >>>>
> >>>> Bernard
> >>>>
> >>> Hi Bernard,
> >>>
> >>> I agree with stop() and close() are only called by primary process,
> >>> but it may not need to add like above.
> >>> Could you please check rte_ethdev.c?
> >>>
> >>> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between processes.
> >>> So we need to initialize of finalize carefully like you said.
> >>>
> >>> - struct rte_eth_dev rte_eth_devices[] This array is per process.
> >>> And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.
> >>>
> >>> All PMDs for physical NIC allocates like above when PMDs are initialized.
> >>> (Even when a process is secondary, initialization function of PMDs
> >>> will be called) But virtual device PMDs allocate rte_eth_dev_data and overwrite 'data'
> >>> variable of rte_eth_devices while initialization.
> >>>
> >>> As a result, primary and secondary process has their own 'rte_eth_dev_data' for a virtual device.
> >>> So I guess all processes need to free it not to leak memory.
> >>>
> >>> Thanks,
> >>> Tetsuya
> >>>
> >> Hi Tetsuya,
> >>
> >> In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro
> PROC_PRIMARY_OR_RET().
> >> So for secondary processes  both functions return without doing anything.
> >> Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?
> >>
> >> For the Physical/Virtual  Functions of the NIC  a lot of the
> >> finalization is done in the  dev->dev_ops->dev_stop() and
> >> dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is
> called, this should probably do nothing for secondary processes  as the dev_stop() and dev_close()
> functions will not have been executed.
> > Hi Bernard,
> >
> > Sorry for my English.
> > I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs.
> > Not a PMDs for virtual functions on NIC.
> >
> > For PMDs like a pcap and af_packet PMDs, all data structures are
> > allocated per processes.
> > (Actually I guess nothing is shared between primary and secondary
> > processes, because rte_eth_dev_data is overwritten by each processes.)
> > So we need to free per process data when detach() is called.
> >
> >> For the Physical/Virtual  Functions of the NIC  the dev_init() is called for both primary and
> secondary processes, however a subset of the function only is executed for secondary processes.
> > Because of above, we probably not be able to add PROC_PRIMARY_OR_RET()
> > to rte_eth_dev_detach().
> > But I agree we should not call rte_eth_dev_detach() for secondary
> > process, if PMDs are like e1000 or ixgbe PMD.
> 
> Correction:
> We should not process rte_eth_dev_detach() for secondary process, if PMDs are like e1000 or ixgbe
> PMD and if primary process hasn't called
> stop() and close() yet.
> 
> Tetsuya
> 
> >
> > To work like above, how about changing drv_flags dynamically in
> > close() callback?
> > For example, when primary process calls rte_eth_dev_close(), a
> > callback of PMD will be called.
> > (In the case of e1000 PMD, eth_em_close() is the callback.)
> >
> > At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the
> > callback.
> > It means if primary process hasn't called close() yet,
> > rte_eth_dev_detach() will do nothing and return error.
> > How about doing like above?
> >
> > Regards,
> > Tetsuya

Hi Tetsuya,
For the e1000, igb and ixgbe PMD's it is probably  simpler to just check for the primary process in the uninit functions and just return without doing anything for secondary processes.

Regards,

Bernard.

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Tetsuya Mukawa]

On 2015/03/16 23:47, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Monday, March 16, 2015 8:57 AM
>> To: Iremonger, Bernard
>> Cc: John W. Linville; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
>>
>>>>>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>>>>>>>       return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +static int
>>>>>>> +rte_pmd_af_packet_devuninit(const char *name) {
>>>>>>> +     struct rte_eth_dev *eth_dev = NULL;
>>>>>>> +     struct pmd_internals *internals;
>>>>>>> +     struct tpacket_req req;
>>>>>>> +
>>>>>>> +     unsigned q;
>>>>>>> +
>>>>>>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
>>>>>>> +                     rte_socket_id());
>>>>>>> +
>>>>>>> +     if (name == NULL)
>>>>>>> +             return -1;
>>>>>> Hi  Tetsuya, John,
>>>>>>
>>>>>> Before detaching a port, the port must be stopped and closed.
>>>>>> The stop and close are only allowed for RTE_PROC_PRIMARY.
>>>>>> Should there be a check for process_type here?
>>>>>>
>>>>>> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>>>         return -EPERM;
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Bernard
>>>>>>
>>>>> Hi Bernard,
>>>>>
>>>>> I agree with stop() and close() are only called by primary process,
>>>>> but it may not need to add like above.
>>>>> Could you please check rte_ethdev.c?
>>>>>
>>>>> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between processes.
>>>>> So we need to initialize of finalize carefully like you said.
>>>>>
>>>>> - struct rte_eth_dev rte_eth_devices[] This array is per process.
>>>>> And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.
>>>>>
>>>>> All PMDs for physical NIC allocates like above when PMDs are initialized.
>>>>> (Even when a process is secondary, initialization function of PMDs
>>>>> will be called) But virtual device PMDs allocate rte_eth_dev_data and overwrite 'data'
>>>>> variable of rte_eth_devices while initialization.
>>>>>
>>>>> As a result, primary and secondary process has their own 'rte_eth_dev_data' for a virtual device.
>>>>> So I guess all processes need to free it not to leak memory.
>>>>>
>>>>> Thanks,
>>>>> Tetsuya
>>>>>
>>>> Hi Tetsuya,
>>>>
>>>> In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro
>> PROC_PRIMARY_OR_RET().
>>>> So for secondary processes  both functions return without doing anything.
>>>> Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?
>>>>
>>>> For the Physical/Virtual  Functions of the NIC  a lot of the
>>>> finalization is done in the  dev->dev_ops->dev_stop() and
>>>> dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is
>> called, this should probably do nothing for secondary processes  as the dev_stop() and dev_close()
>> functions will not have been executed.
>>> Hi Bernard,
>>>
>>> Sorry for my English.
>>> I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs.
>>> Not a PMDs for virtual functions on NIC.
>>>
>>> For PMDs like a pcap and af_packet PMDs, all data structures are
>>> allocated per processes.
>>> (Actually I guess nothing is shared between primary and secondary
>>> processes, because rte_eth_dev_data is overwritten by each processes.)
>>> So we need to free per process data when detach() is called.
>>>
>>>> For the Physical/Virtual  Functions of the NIC  the dev_init() is called for both primary and
>> secondary processes, however a subset of the function only is executed for secondary processes.
>>> Because of above, we probably not be able to add PROC_PRIMARY_OR_RET()
>>> to rte_eth_dev_detach().
>>> But I agree we should not call rte_eth_dev_detach() for secondary
>>> process, if PMDs are like e1000 or ixgbe PMD.
>> Correction:
>> We should not process rte_eth_dev_detach() for secondary process, if PMDs are like e1000 or ixgbe
>> PMD and if primary process hasn't called
>> stop() and close() yet.
>>
>> Tetsuya
>>
>>> To work like above, how about changing drv_flags dynamically in
>>> close() callback?
>>> For example, when primary process calls rte_eth_dev_close(), a
>>> callback of PMD will be called.
>>> (In the case of e1000 PMD, eth_em_close() is the callback.)
>>>
>>> At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the
>>> callback.
>>> It means if primary process hasn't called close() yet,
>>> rte_eth_dev_detach() will do nothing and return error.
>>> How about doing like above?
>>>
>>> Regards,
>>> Tetsuya
> Hi Tetsuya,
> For the e1000, igb and ixgbe PMD's it is probably  simpler to just check for the primary process in the uninit functions and just return without doing anything for secondary processes.

Thanks for clarifying.
In the case, is it okay for you to add PROC_PRIMARY_OR_RET() in e1000,
igb and ixgbe PMD code?
If it's okay, we may be able to ACK this patch. :)

Regards,
Tetsuya

>
> Regards,
>
> Bernard.
>
>
>
>
>

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Iremonger, Bernard]

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, March 17, 2015 3:43 AM
> To: Iremonger, Bernard
> Cc: John W. Linville; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
> 
> On 2015/03/16 23:47, Iremonger, Bernard wrote:
> >
> >> -----Original Message-----
> >> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> >> Sent: Monday, March 16, 2015 8:57 AM
> >> To: Iremonger, Bernard
> >> Cc: John W. Linville; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
> >>
> >>>>>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char
> *params)
> >>>>>>>       return 0;
> >>>>>>>  }
> >>>>>>>
> >>>>>>> +static int
> >>>>>>> +rte_pmd_af_packet_devuninit(const char *name) {
> >>>>>>> +     struct rte_eth_dev *eth_dev = NULL;
> >>>>>>> +     struct pmd_internals *internals;
> >>>>>>> +     struct tpacket_req req;
> >>>>>>> +
> >>>>>>> +     unsigned q;
> >>>>>>> +
> >>>>>>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
> >>>>>>> +                     rte_socket_id());
> >>>>>>> +
> >>>>>>> +     if (name == NULL)
> >>>>>>> +             return -1;
> >>>>>> Hi  Tetsuya, John,
> >>>>>>
> >>>>>> Before detaching a port, the port must be stopped and closed.
> >>>>>> The stop and close are only allowed for RTE_PROC_PRIMARY.
> >>>>>> Should there be a check for process_type here?
> >>>>>>
> >>>>>> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>>>>>         return -EPERM;
> >>>>>>
> >>>>>> Regards,
> >>>>>>
> >>>>>> Bernard
> >>>>>>
> >>>>> Hi Bernard,
> >>>>>
> >>>>> I agree with stop() and close() are only called by primary
> >>>>> process, but it may not need to add like above.
> >>>>> Could you please check rte_ethdev.c?
> >>>>>
> >>>>> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between processes.
> >>>>> So we need to initialize of finalize carefully like you said.
> >>>>>
> >>>>> - struct rte_eth_dev rte_eth_devices[] This array is per process.
> >>>>> And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.
> >>>>>
> >>>>> All PMDs for physical NIC allocates like above when PMDs are initialized.
> >>>>> (Even when a process is secondary, initialization function of PMDs
> >>>>> will be called) But virtual device PMDs allocate rte_eth_dev_data and overwrite 'data'
> >>>>> variable of rte_eth_devices while initialization.
> >>>>>
> >>>>> As a result, primary and secondary process has their own 'rte_eth_dev_data' for a virtual
> device.
> >>>>> So I guess all processes need to free it not to leak memory.
> >>>>>
> >>>>> Thanks,
> >>>>> Tetsuya
> >>>>>
> >>>> Hi Tetsuya,
> >>>>
> >>>> In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro
> >> PROC_PRIMARY_OR_RET().
> >>>> So for secondary processes  both functions return without doing anything.
> >>>> Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?
> >>>>
> >>>> For the Physical/Virtual  Functions of the NIC  a lot of the
> >>>> finalization is done in the  dev->dev_ops->dev_stop() and
> >>>> dev->dev_ops->dev_close() functions. To complete the finalization
> >>>> dev->the dev_uninit() function is
> >> called, this should probably do nothing for secondary processes  as
> >> the dev_stop() and dev_close() functions will not have been executed.
> >>> Hi Bernard,
> >>>
> >>> Sorry for my English.
> >>> I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs.
> >>> Not a PMDs for virtual functions on NIC.
> >>>
> >>> For PMDs like a pcap and af_packet PMDs, all data structures are
> >>> allocated per processes.
> >>> (Actually I guess nothing is shared between primary and secondary
> >>> processes, because rte_eth_dev_data is overwritten by each
> >>> processes.) So we need to free per process data when detach() is called.
> >>>
> >>>> For the Physical/Virtual  Functions of the NIC  the dev_init() is
> >>>> called for both primary and
> >> secondary processes, however a subset of the function only is executed for secondary processes.
> >>> Because of above, we probably not be able to add
> >>> PROC_PRIMARY_OR_RET() to rte_eth_dev_detach().
> >>> But I agree we should not call rte_eth_dev_detach() for secondary
> >>> process, if PMDs are like e1000 or ixgbe PMD.
> >> Correction:
> >> We should not process rte_eth_dev_detach() for secondary process, if
> >> PMDs are like e1000 or ixgbe PMD and if primary process hasn't called
> >> stop() and close() yet.
> >>
> >> Tetsuya
> >>
> >>> To work like above, how about changing drv_flags dynamically in
> >>> close() callback?
> >>> For example, when primary process calls rte_eth_dev_close(), a
> >>> callback of PMD will be called.
> >>> (In the case of e1000 PMD, eth_em_close() is the callback.)
> >>>
> >>> At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the
> >>> callback.
> >>> It means if primary process hasn't called close() yet,
> >>> rte_eth_dev_detach() will do nothing and return error.
> >>> How about doing like above?
> >>>
> >>> Regards,
> >>> Tetsuya
> > Hi Tetsuya,
> > For the e1000, igb and ixgbe PMD's it is probably  simpler to just check for the primary process in the
> uninit functions and just return without doing anything for secondary processes.
> 
> Thanks for clarifying.
> In the case, is it okay for you to add PROC_PRIMARY_OR_RET() in e1000, igb and ixgbe PMD code?
> If it's okay, we may be able to ACK this patch. :)
> 
> Regards,
> Tetsuya
>

Hi Tetsuya,

I will add the process type check in the e1000, igb amd ixgbe PMD code.

Regards,

Bernard.

 

Re: [dpdk-dev] [RFC] af_packet: support port hotplug

[Iremonger, Bernard]

-----Original Message-----
From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp] 
Sent: Tuesday, March 17, 2015 3:43 AM
To: Iremonger, Bernard
Cc: John W. Linville; dev@dpdk.org
Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug

On 2015/03/16 23:47, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Monday, March 16, 2015 8:57 AM
>> To: Iremonger, Bernard
>> Cc: John W. Linville; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug
>>
>>>>>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>>>>>>>       return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +static int
>>>>>>> +rte_pmd_af_packet_devuninit(const char *name) {
>>>>>>> +     struct rte_eth_dev *eth_dev = NULL;
>>>>>>> +     struct pmd_internals *internals;
>>>>>>> +     struct tpacket_req req;
>>>>>>> +
>>>>>>> +     unsigned q;
>>>>>>> +
>>>>>>> +     RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u\n",
>>>>>>> +                     rte_socket_id());
>>>>>>> +
>>>>>>> +     if (name == NULL)
>>>>>>> +             return -1;
>>>>>> Hi  Tetsuya, John,
>>>>>>
>>>>>> Before detaching a port, the port must be stopped and closed.
>>>>>> The stop and close are only allowed for RTE_PROC_PRIMARY.
>>>>>> Should there be a check for process_type here?
>>>>>>
>>>>>> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>>>         return -EPERM;
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Bernard
>>>>>>
>>>>> Hi Bernard,
>>>>>
>>>>> I agree with stop() and close() are only called by primary 
>>>>> process, but it may not need to add like above.
>>>>> Could you please check rte_ethdev.c?
>>>>>
>>>>> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared between processes.
>>>>> So we need to initialize of finalize carefully like you said.
>>>>>
>>>>> - struct rte_eth_dev rte_eth_devices[] This array is per process.
>>>>> And 'data' variable of this structure indicates a pointer of rte_eth_dev_data.
>>>>>
>>>>> All PMDs for physical NIC allocates like above when PMDs are initialized.
>>>>> (Even when a process is secondary, initialization function of PMDs 
>>>>> will be called) But virtual device PMDs allocate rte_eth_dev_data and overwrite 'data'
>>>>> variable of rte_eth_devices while initialization.
>>>>>
>>>>> As a result, primary and secondary process has their own 'rte_eth_dev_data' for a virtual device.
>>>>> So I guess all processes need to free it not to leak memory.
>>>>>
>>>>> Thanks,
>>>>> Tetsuya
>>>>>
>>>> Hi Tetsuya,
>>>>
>>>> In rte_ethdev.c   both rte_eth_dev_stop() and rte_eth_dev_close()  use the macro
>> PROC_PRIMARY_OR_RET().
>>>> So for secondary processes  both functions return without doing anything.
>>>> Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ?
>>>>
>>>> For the Physical/Virtual  Functions of the NIC  a lot of the 
>>>> finalization is done in the  dev->dev_ops->dev_stop() and
>>>> dev->dev_ops->dev_close() functions. To complete the finalization 
>>>> dev->the dev_uninit() function is
>> called, this should probably do nothing for secondary processes  as 
>> the dev_stop() and dev_close() functions will not have been executed.
>>> Hi Bernard,
>>>
>>> Sorry for my English.
>>> I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs.
>>> Not a PMDs for virtual functions on NIC.
>>>
>>> For PMDs like a pcap and af_packet PMDs, all data structures are 
>>> allocated per processes.
>>> (Actually I guess nothing is shared between primary and secondary 
>>> processes, because rte_eth_dev_data is overwritten by each 
>>> processes.) So we need to free per process data when detach() is called.
>>>
>>>> For the Physical/Virtual  Functions of the NIC  the dev_init() is 
>>>> called for both primary and
>> secondary processes, however a subset of the function only is executed for secondary processes.
>>> Because of above, we probably not be able to add 
>>> PROC_PRIMARY_OR_RET() to rte_eth_dev_detach().
>>> But I agree we should not call rte_eth_dev_detach() for secondary 
>>> process, if PMDs are like e1000 or ixgbe PMD.
>> Correction:
>> We should not process rte_eth_dev_detach() for secondary process, if 
>> PMDs are like e1000 or ixgbe PMD and if primary process hasn't called
>> stop() and close() yet.
>>
>> Tetsuya
>>
>>> To work like above, how about changing drv_flags dynamically in
>>> close() callback?
>>> For example, when primary process calls rte_eth_dev_close(), a 
>>> callback of PMD will be called.
>>> (In the case of e1000 PMD, eth_em_close() is the callback.)
>>>
>>> At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the 
>>> callback.
>>> It means if primary process hasn't called close() yet,
>>> rte_eth_dev_detach() will do nothing and return error.
>>> How about doing like above?
>>>
>>> Regards,
>>> Tetsuya
> Hi Tetsuya,
> For the e1000, igb and ixgbe PMD's it is probably  simpler to just check for the primary process in the uninit functions and just return without doing anything for secondary processes.

Thanks for clarifying.
In the case, is it okay for you to add PROC_PRIMARY_OR_RET() in e1000, igb and ixgbe PMD code?
If it's okay, we may be able to ACK this patch. :)

Regards,
Tetsuya

>
> Regards,
>
> Bernard.

Hi John,

The code in lib/librte_pmd_af_packet  has been moved to drivers/net/af_packet.
This patch will need to be rebased to use the drivers/net/af_packet  directory.

Regards,

Bernard.