DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/enic: fix multi-process operation
@ 2017-09-11 18:58 John Daley
  2017-09-12 13:53 ` Aaron Conole
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: John Daley @ 2017-09-11 18:58 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, John Daley, stable

- Use rte_malloc() instead of malloc() for the per device 'vdev' structure
  so that it can be shared across processes.
- Only initialize the device if the process type is RTE_PROC_PRIMARY
- Only allow the primary process to do queue setup, start/stop, promisc
  allmulticast, mac add/del, mtu.

Fixes: fefed3d1e62c ("enic: new driver")
Cc: stable@dpdk.org

Signed-off-by: John Daley <johndale@cisco.com>
---
 doc/guides/nics/features/enic.ini |  1 +
 drivers/net/enic/base/vnic_dev.c  | 10 +++++++--
 drivers/net/enic/enic_ethdev.c    | 43 +++++++++++++++++++++++++++++++++++++++
 drivers/net/enic/enic_main.c      |  7 +++++++
 4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/doc/guides/nics/features/enic.ini b/doc/guides/nics/features/enic.ini
index 0de3ef53c..498341f07 100644
--- a/doc/guides/nics/features/enic.ini
+++ b/doc/guides/nics/features/enic.ini
@@ -25,6 +25,7 @@ L3 checksum offload  = Y
 L4 checksum offload  = Y
 Packet type parsing  = Y
 Basic stats          = Y
+Multiprocess aware   = Y
 BSD nic_uio          = Y
 Linux UIO            = Y
 Linux VFIO           = Y
diff --git a/drivers/net/enic/base/vnic_dev.c b/drivers/net/enic/base/vnic_dev.c
index 49b36555b..162e9c2f2 100644
--- a/drivers/net/enic/base/vnic_dev.c
+++ b/drivers/net/enic/base/vnic_dev.c
@@ -1063,7 +1063,7 @@ void vnic_dev_unregister(struct vnic_dev *vdev)
 			vdev->free_consistent(vdev->priv,
 				sizeof(struct vnic_devcmd_fw_info),
 				vdev->fw_info, vdev->fw_info_pa);
-		kfree(vdev);
+		rte_free(vdev);
 	}
 }
 
@@ -1072,7 +1072,13 @@ struct vnic_dev *vnic_dev_register(struct vnic_dev *vdev,
 	unsigned int num_bars)
 {
 	if (!vdev) {
-		vdev = kzalloc(sizeof(struct vnic_dev), GFP_ATOMIC);
+		char name[NAME_MAX];
+		snprintf((char *)name, sizeof(name), "%s-vnic",
+			  pdev->device.name);
+		vdev = (struct vnic_dev *)rte_zmalloc_socket(name,
+					sizeof(struct vnic_dev),
+					RTE_CACHE_LINE_SIZE,
+					pdev->device.numa_node);
 		if (!vdev)
 			return NULL;
 	}
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index da8fec2d0..33a3f87e2 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
 static void enicpmd_dev_tx_queue_release(void *txq)
 {
 	ENICPMD_FUNC_TRACE();
+
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	enic_free_wq(txq);
 }
 
@@ -196,6 +200,9 @@ static int enicpmd_dev_tx_queue_setup(struct rte_eth_dev *eth_dev,
 	int ret;
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
+
 	ENICPMD_FUNC_TRACE();
 	if (queue_idx >= ENIC_WQ_MAX) {
 		dev_err(enic,
@@ -272,6 +279,10 @@ static int enicpmd_dev_rx_queue_stop(struct rte_eth_dev *eth_dev,
 static void enicpmd_dev_rx_queue_release(void *rxq)
 {
 	ENICPMD_FUNC_TRACE();
+
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	enic_free_rq(rxq);
 }
 
@@ -310,6 +321,10 @@ static int enicpmd_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
 	struct enic *enic = pmd_priv(eth_dev);
 
 	ENICPMD_FUNC_TRACE();
+
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
+
 	/* With Rx scatter support, two RQs are now used on VIC per RQ used
 	 * by the application.
 	 */
@@ -378,6 +393,9 @@ static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev)
 	int ret;
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
+
 	ENICPMD_FUNC_TRACE();
 	ret = enic_set_vnic_res(enic);
 	if (ret) {
@@ -404,6 +422,9 @@ static int enicpmd_dev_start(struct rte_eth_dev *eth_dev)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
+
 	ENICPMD_FUNC_TRACE();
 	return enic_enable(enic);
 }
@@ -416,6 +437,9 @@ static void enicpmd_dev_stop(struct rte_eth_dev *eth_dev)
 	struct rte_eth_link link;
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	ENICPMD_FUNC_TRACE();
 	enic_disable(enic);
 	memset(&link, 0, sizeof(link));
@@ -513,7 +537,11 @@ static void enicpmd_dev_promiscuous_enable(struct rte_eth_dev *eth_dev)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	ENICPMD_FUNC_TRACE();
+
 	enic->promisc = 1;
 	enic_add_packet_filter(enic);
 }
@@ -522,6 +550,9 @@ static void enicpmd_dev_promiscuous_disable(struct rte_eth_dev *eth_dev)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	ENICPMD_FUNC_TRACE();
 	enic->promisc = 0;
 	enic_add_packet_filter(enic);
@@ -531,6 +562,9 @@ static void enicpmd_dev_allmulticast_enable(struct rte_eth_dev *eth_dev)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	ENICPMD_FUNC_TRACE();
 	enic->allmulti = 1;
 	enic_add_packet_filter(enic);
@@ -540,6 +574,9 @@ static void enicpmd_dev_allmulticast_disable(struct rte_eth_dev *eth_dev)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	ENICPMD_FUNC_TRACE();
 	enic->allmulti = 0;
 	enic_add_packet_filter(enic);
@@ -551,6 +588,9 @@ static int enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
+
 	ENICPMD_FUNC_TRACE();
 	return enic_set_mac_address(enic, mac_addr->addr_bytes);
 }
@@ -559,6 +599,9 @@ static void enicpmd_remove_mac_addr(struct rte_eth_dev *eth_dev, uint32_t index)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
 	ENICPMD_FUNC_TRACE();
 	enic_del_mac_address(enic, index);
 }
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 1d956cd94..9b0439b96 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -1181,6 +1181,9 @@ int enic_set_mtu(struct enic *enic, uint16_t new_mtu)
 	old_mtu = eth_dev->data->mtu;
 	config_mtu = enic->config.mtu;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
+
 	if (new_mtu > enic->max_mtu) {
 		dev_err(enic,
 			"MTU not updated: requested (%u) greater than max (%u)\n",
@@ -1332,6 +1335,10 @@ int enic_probe(struct enic *enic)
 
 	dev_debug(enic, " Initializing ENIC PMD\n");
 
+	/* if this is a secondary process the hardware is already initialized */
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return 0;
+
 	enic->bar0.vaddr = (void *)pdev->mem_resource[0].addr;
 	enic->bar0.len = pdev->mem_resource[0].len;
 
-- 
2.12.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
  2017-09-11 18:58 [dpdk-dev] [PATCH] net/enic: fix multi-process operation John Daley
@ 2017-09-12 13:53 ` Aaron Conole
  2017-09-18 21:27 ` Ferruh Yigit
  2017-09-22 16:50 ` Ferruh Yigit
  2 siblings, 0 replies; 8+ messages in thread
From: Aaron Conole @ 2017-09-12 13:53 UTC (permalink / raw)
  To: John Daley; +Cc: ferruh.yigit, dev, stable

John Daley <johndale@cisco.com> writes:

> - Use rte_malloc() instead of malloc() for the per device 'vdev' structure
>   so that it can be shared across processes.
> - Only initialize the device if the process type is RTE_PROC_PRIMARY
> - Only allow the primary process to do queue setup, start/stop, promisc
>   allmulticast, mac add/del, mtu.
>
> Fixes: fefed3d1e62c ("enic: new driver")
> Cc: stable@dpdk.org
>
> Signed-off-by: John Daley <johndale@cisco.com>
> ---

I was first considering that this should be multiple patches, but then I
realized that it wouldn't actually make sense (there's no way to do it
and keep the driver from being broken w.r.t. primary/secondary).

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  doc/guides/nics/features/enic.ini |  1 +
>  drivers/net/enic/base/vnic_dev.c  | 10 +++++++--
>  drivers/net/enic/enic_ethdev.c    | 43 +++++++++++++++++++++++++++++++++++++++
>  drivers/net/enic/enic_main.c      |  7 +++++++
>  4 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/doc/guides/nics/features/enic.ini b/doc/guides/nics/features/enic.ini
> index 0de3ef53c..498341f07 100644
> --- a/doc/guides/nics/features/enic.ini
> +++ b/doc/guides/nics/features/enic.ini
> @@ -25,6 +25,7 @@ L3 checksum offload  = Y
>  L4 checksum offload  = Y
>  Packet type parsing  = Y
>  Basic stats          = Y
> +Multiprocess aware   = Y
>  BSD nic_uio          = Y
>  Linux UIO            = Y
>  Linux VFIO           = Y
> diff --git a/drivers/net/enic/base/vnic_dev.c b/drivers/net/enic/base/vnic_dev.c
> index 49b36555b..162e9c2f2 100644
> --- a/drivers/net/enic/base/vnic_dev.c
> +++ b/drivers/net/enic/base/vnic_dev.c
> @@ -1063,7 +1063,7 @@ void vnic_dev_unregister(struct vnic_dev *vdev)
>  			vdev->free_consistent(vdev->priv,
>  				sizeof(struct vnic_devcmd_fw_info),
>  				vdev->fw_info, vdev->fw_info_pa);
> -		kfree(vdev);
> +		rte_free(vdev);
>  	}
>  }
>  
> @@ -1072,7 +1072,13 @@ struct vnic_dev *vnic_dev_register(struct vnic_dev *vdev,
>  	unsigned int num_bars)
>  {
>  	if (!vdev) {
> -		vdev = kzalloc(sizeof(struct vnic_dev), GFP_ATOMIC);
> +		char name[NAME_MAX];
> +		snprintf((char *)name, sizeof(name), "%s-vnic",
> +			  pdev->device.name);
> +		vdev = (struct vnic_dev *)rte_zmalloc_socket(name,
> +					sizeof(struct vnic_dev),
> +					RTE_CACHE_LINE_SIZE,
> +					pdev->device.numa_node);
>  		if (!vdev)
>  			return NULL;
>  	}
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index da8fec2d0..33a3f87e2 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
>  static void enicpmd_dev_tx_queue_release(void *txq)
>  {
>  	ENICPMD_FUNC_TRACE();
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	enic_free_wq(txq);
>  }
>  
> @@ -196,6 +200,9 @@ static int enicpmd_dev_tx_queue_setup(struct rte_eth_dev *eth_dev,
>  	int ret;
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
> +
>  	ENICPMD_FUNC_TRACE();
>  	if (queue_idx >= ENIC_WQ_MAX) {
>  		dev_err(enic,
> @@ -272,6 +279,10 @@ static int enicpmd_dev_rx_queue_stop(struct rte_eth_dev *eth_dev,
>  static void enicpmd_dev_rx_queue_release(void *rxq)
>  {
>  	ENICPMD_FUNC_TRACE();
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	enic_free_rq(rxq);
>  }
>  
> @@ -310,6 +321,10 @@ static int enicpmd_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
>  	struct enic *enic = pmd_priv(eth_dev);
>  
>  	ENICPMD_FUNC_TRACE();
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
> +
>  	/* With Rx scatter support, two RQs are now used on VIC per RQ used
>  	 * by the application.
>  	 */
> @@ -378,6 +393,9 @@ static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev)
>  	int ret;
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
> +
>  	ENICPMD_FUNC_TRACE();
>  	ret = enic_set_vnic_res(enic);
>  	if (ret) {
> @@ -404,6 +422,9 @@ static int enicpmd_dev_start(struct rte_eth_dev *eth_dev)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
> +
>  	ENICPMD_FUNC_TRACE();
>  	return enic_enable(enic);
>  }
> @@ -416,6 +437,9 @@ static void enicpmd_dev_stop(struct rte_eth_dev *eth_dev)
>  	struct rte_eth_link link;
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	ENICPMD_FUNC_TRACE();
>  	enic_disable(enic);
>  	memset(&link, 0, sizeof(link));
> @@ -513,7 +537,11 @@ static void enicpmd_dev_promiscuous_enable(struct rte_eth_dev *eth_dev)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	ENICPMD_FUNC_TRACE();
> +
>  	enic->promisc = 1;
>  	enic_add_packet_filter(enic);
>  }
> @@ -522,6 +550,9 @@ static void enicpmd_dev_promiscuous_disable(struct rte_eth_dev *eth_dev)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	ENICPMD_FUNC_TRACE();
>  	enic->promisc = 0;
>  	enic_add_packet_filter(enic);
> @@ -531,6 +562,9 @@ static void enicpmd_dev_allmulticast_enable(struct rte_eth_dev *eth_dev)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	ENICPMD_FUNC_TRACE();
>  	enic->allmulti = 1;
>  	enic_add_packet_filter(enic);
> @@ -540,6 +574,9 @@ static void enicpmd_dev_allmulticast_disable(struct rte_eth_dev *eth_dev)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	ENICPMD_FUNC_TRACE();
>  	enic->allmulti = 0;
>  	enic_add_packet_filter(enic);
> @@ -551,6 +588,9 @@ static int enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
> +
>  	ENICPMD_FUNC_TRACE();
>  	return enic_set_mac_address(enic, mac_addr->addr_bytes);
>  }
> @@ -559,6 +599,9 @@ static void enicpmd_remove_mac_addr(struct rte_eth_dev *eth_dev, uint32_t index)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	ENICPMD_FUNC_TRACE();
>  	enic_del_mac_address(enic, index);
>  }
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 1d956cd94..9b0439b96 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -1181,6 +1181,9 @@ int enic_set_mtu(struct enic *enic, uint16_t new_mtu)
>  	old_mtu = eth_dev->data->mtu;
>  	config_mtu = enic->config.mtu;
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
> +
>  	if (new_mtu > enic->max_mtu) {
>  		dev_err(enic,
>  			"MTU not updated: requested (%u) greater than max (%u)\n",
> @@ -1332,6 +1335,10 @@ int enic_probe(struct enic *enic)
>  
>  	dev_debug(enic, " Initializing ENIC PMD\n");
>  
> +	/* if this is a secondary process the hardware is already initialized */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
>  	enic->bar0.vaddr = (void *)pdev->mem_resource[0].addr;
>  	enic->bar0.len = pdev->mem_resource[0].len;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
  2017-09-11 18:58 [dpdk-dev] [PATCH] net/enic: fix multi-process operation John Daley
  2017-09-12 13:53 ` Aaron Conole
@ 2017-09-18 21:27 ` Ferruh Yigit
  2017-09-18 22:25   ` Thomas Monjalon
  2017-09-22 16:50 ` Ferruh Yigit
  2 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2017-09-18 21:27 UTC (permalink / raw)
  To: John Daley; +Cc: dev, stable, Sergio Gonzalez Monroy, Thomas Monjalon

On 9/11/2017 7:58 PM, John Daley wrote:
> - Use rte_malloc() instead of malloc() for the per device 'vdev' structure
>   so that it can be shared across processes.
> - Only initialize the device if the process type is RTE_PROC_PRIMARY
> - Only allow the primary process to do queue setup, start/stop, promisc
>   allmulticast, mac add/del, mtu.
> 
> Fixes: fefed3d1e62c ("enic: new driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: John Daley <johndale@cisco.com>

<...>

> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index da8fec2d0..33a3f87e2 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
>  static void enicpmd_dev_tx_queue_release(void *txq)
>  {
>  	ENICPMD_FUNC_TRACE();
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +

Hi John,

I am not sure about these updates. Agree that these functions should
know process type, but all others PMDs don't do this.

Added a few more people for comment, but as far I understand its
application responsibility to NOT call these functions if it is
secondary process.

For device init/uninit, that is part of eal_init() and have to be called
both for primary and secondary process and PMD needs to protect it, for
other functions application's responsibility.


>  	enic_free_wq(txq);
>  }
>  
> @@ -196,6 +200,9 @@ static int enicpmd_dev_tx_queue_setup(struct rte_eth_dev *eth_dev,
>  	int ret;
>  	struct enic *enic = pmd_priv(eth_dev);
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
> +
>  	ENICPMD_FUNC_TRACE();
>  	if (queue_idx >= ENIC_WQ_MAX) {
>  		dev_err(enic,
> @@ -272,6 +279,10 @@ static int enicpmd_dev_rx_queue_stop(struct rte_eth_dev *eth_dev,
>  static void enicpmd_dev_rx_queue_release(void *rxq)
>  {
>  	ENICPMD_FUNC_TRACE();
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
>  	enic_free_rq(rxq);
>  }
>  

<...>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
  2017-09-18 21:27 ` Ferruh Yigit
@ 2017-09-18 22:25   ` Thomas Monjalon
  2017-09-19  5:31     ` John Daley (johndale)
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2017-09-18 22:25 UTC (permalink / raw)
  To: John Daley; +Cc: Ferruh Yigit, dev, Sergio Gonzalez Monroy

18/09/2017 23:27, Ferruh Yigit:
> On 9/11/2017 7:58 PM, John Daley wrote:
> > - Use rte_malloc() instead of malloc() for the per device 'vdev' structure
> >   so that it can be shared across processes.
> > - Only initialize the device if the process type is RTE_PROC_PRIMARY
> > - Only allow the primary process to do queue setup, start/stop, promisc
> >   allmulticast, mac add/del, mtu.
[...]
> > --- a/drivers/net/enic/enic_ethdev.c
> > +++ b/drivers/net/enic/enic_ethdev.c
> > @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
> >  static void enicpmd_dev_tx_queue_release(void *txq)
> >  {
> >  	ENICPMD_FUNC_TRACE();
> > +
> > +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return;
> > +
> 
> Hi John,
> 
> I am not sure about these updates. Agree that these functions should
> know process type, but all others PMDs don't do this.
> 
> Added a few more people for comment, but as far I understand its
> application responsibility to NOT call these functions if it is
> secondary process.
> 
> For device init/uninit, that is part of eal_init() and have to be called
> both for primary and secondary process and PMD needs to protect it, for
> other functions application's responsibility.

Yes for now it is the policy.
But it is a gray area and it could be clearer with my "ownership proposal":
	http://dpdk.org/ml/archives/dev/2017-September/074656.html
A secondary process could manage the ports it owns.

Feel free to comment the proposal.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
  2017-09-18 22:25   ` Thomas Monjalon
@ 2017-09-19  5:31     ` John Daley (johndale)
  2017-09-19 12:58       ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: John Daley (johndale) @ 2017-09-19  5:31 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Ferruh Yigit, dev, Sergio Gonzalez Monroy


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, September 18, 2017 3:25 PM
> To: John Daley (johndale) <johndale@cisco.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Sergio Gonzalez
> Monroy <sergio.gonzalez.monroy@intel.com>
> Subject: Re: [PATCH] net/enic: fix multi-process operation
> 
> 18/09/2017 23:27, Ferruh Yigit:
> > On 9/11/2017 7:58 PM, John Daley wrote:
> > > - Use rte_malloc() instead of malloc() for the per device 'vdev' structure
> > >   so that it can be shared across processes.
> > > - Only initialize the device if the process type is RTE_PROC_PRIMARY
> > > - Only allow the primary process to do queue setup, start/stop, promisc
> > >   allmulticast, mac add/del, mtu.
> [...]
> > > --- a/drivers/net/enic/enic_ethdev.c
> > > +++ b/drivers/net/enic/enic_ethdev.c
> > > @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
> > > *dev,  static void enicpmd_dev_tx_queue_release(void *txq)  {
> > >  	ENICPMD_FUNC_TRACE();
> > > +
> > > +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > > +		return;
> > > +
> >
> > Hi John,
> >
> > I am not sure about these updates. Agree that these functions should
> > know process type, but all others PMDs don't do this.

I looked at mlx5 and it does something similar with its mlx5_is_secondary() in functions that modify fields in its shared private structure.

> >
> > Added a few more people for comment, but as far I understand its
> > application responsibility to NOT call these functions if it is
> > secondary process.
> >
> > For device init/uninit, that is part of eal_init() and have to be
> > called both for primary and secondary process and PMD needs to protect
> > it, for other functions application's responsibility.

I put the checks into the PMD because I didn't want to trust the app and I didn't see checks in the library functions. I assumed that is why it was done in mlx5. I was afraid that the secondary may try to write fields in the shared structure and cause some silent bad behavior, like if secondary sets the MTU then the primary does, then secondary assumes it was different than what it is, or something like that.
> 
> Yes for now it is the policy.
> But it is a gray area and it could be clearer with my "ownership proposal":
> 	http://dpdk.org/ml/archives/dev/2017-September/074656.html
> A secondary process could manage the ports it owns.

I think the ownership concept would help make DPDK more flexible and potentially cleaner. Perhaps ownership checks could be pushed the lib functions, like rte_eth_dev_set_mtu(), and remove all such checks in the PMD(s). 
> 
> Feel free to comment the proposal.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
  2017-09-19  5:31     ` John Daley (johndale)
@ 2017-09-19 12:58       ` Ferruh Yigit
  2017-09-22 13:02         ` John Daley (johndale)
  0 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2017-09-19 12:58 UTC (permalink / raw)
  To: John Daley (johndale), Thomas Monjalon
  Cc: dev, Sergio Gonzalez Monroy, Adrien Mazarguil, Nelio Laranjeiro,
	Ananyev, Konstantin

On 9/19/2017 6:31 AM, John Daley (johndale) wrote:
> 
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Monday, September 18, 2017 3:25 PM
>> To: John Daley (johndale) <johndale@cisco.com>
>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Sergio Gonzalez
>> Monroy <sergio.gonzalez.monroy@intel.com>
>> Subject: Re: [PATCH] net/enic: fix multi-process operation
>>
>> 18/09/2017 23:27, Ferruh Yigit:
>>> On 9/11/2017 7:58 PM, John Daley wrote:
>>>> - Use rte_malloc() instead of malloc() for the per device 'vdev' structure
>>>>   so that it can be shared across processes.
>>>> - Only initialize the device if the process type is RTE_PROC_PRIMARY
>>>> - Only allow the primary process to do queue setup, start/stop, promisc
>>>>   allmulticast, mac add/del, mtu.
>> [...]
>>>> --- a/drivers/net/enic/enic_ethdev.c
>>>> +++ b/drivers/net/enic/enic_ethdev.c
>>>> @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
>>>> *dev,  static void enicpmd_dev_tx_queue_release(void *txq)  {
>>>>  	ENICPMD_FUNC_TRACE();
>>>> +
>>>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>> +		return;
>>>> +
>>>
>>> Hi John,
>>>
>>> I am not sure about these updates. Agree that these functions should
>>> know process type, but all others PMDs don't do this.
> 
> I looked at mlx5 and it does something similar with its mlx5_is_secondary() in functions that modify fields in its shared private structure.

Right, mlx5 also has these checks.

> 
>>>
>>> Added a few more people for comment, but as far I understand its
>>> application responsibility to NOT call these functions if it is
>>> secondary process.
>>>
>>> For device init/uninit, that is part of eal_init() and have to be
>>> called both for primary and secondary process and PMD needs to protect
>>> it, for other functions application's responsibility.
> 
> I put the checks into the PMD because I didn't want to trust the app and I didn't see checks in the library functions. I assumed that is why it was done in mlx5. I was afraid that the secondary may try to write fields in the shared structure and cause some silent bad behavior, like if secondary sets the MTU then the primary does, then secondary assumes it was different than what it is, or something like that.

The set values are in the shared memory, so a variable set by secondary
will be visible to primary, via versa. Of course a synchronization
required between primary and secondary processes.

Also why secondary shouldn't be allowed to do some work, like start/stop
the port?

I believe this should be application level concern, at worst libraries
but not drivers.

>>
>> Yes for now it is the policy.
>> But it is a gray area and it could be clearer with my "ownership proposal":
>> 	http://dpdk.org/ml/archives/dev/2017-September/074656.html
>> A secondary process could manage the ports it owns.
> 
> I think the ownership concept would help make DPDK more flexible and potentially cleaner. Perhaps ownership checks could be pushed the lib functions, like rte_eth_dev_set_mtu(), and remove all such checks in the PMD(s). 
>>
>> Feel free to comment the proposal.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
  2017-09-19 12:58       ` Ferruh Yigit
@ 2017-09-22 13:02         ` John Daley (johndale)
  0 siblings, 0 replies; 8+ messages in thread
From: John Daley (johndale) @ 2017-09-22 13:02 UTC (permalink / raw)
  To: Ferruh Yigit, Thomas Monjalon
  Cc: dev, Sergio Gonzalez Monroy, Adrien Mazarguil, Nelio Laranjeiro,
	Ananyev, Konstantin

Hi Ferruh,
-johnd

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, September 19, 2017 5:59 AM
> To: John Daley (johndale) <johndale@cisco.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org; Sergio Gonzalez Monroy
> <sergio.gonzalez.monroy@intel.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; Nelio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
> 
> On 9/19/2017 6:31 AM, John Daley (johndale) wrote:
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >> Sent: Monday, September 18, 2017 3:25 PM
> >> To: John Daley (johndale) <johndale@cisco.com>
> >> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Sergio
> >> Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> >> Subject: Re: [PATCH] net/enic: fix multi-process operation
> >>
> >> 18/09/2017 23:27, Ferruh Yigit:
> >>> On 9/11/2017 7:58 PM, John Daley wrote:
> >>>> - Use rte_malloc() instead of malloc() for the per device 'vdev'
> structure
> >>>>   so that it can be shared across processes.
> >>>> - Only initialize the device if the process type is
> >>>> RTE_PROC_PRIMARY
> >>>> - Only allow the primary process to do queue setup, start/stop, promisc
> >>>>   allmulticast, mac add/del, mtu.
> >> [...]
> >>>> --- a/drivers/net/enic/enic_ethdev.c
> >>>> +++ b/drivers/net/enic/enic_ethdev.c
> >>>> @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
> >>>> *dev,  static void enicpmd_dev_tx_queue_release(void *txq)  {
> >>>>  	ENICPMD_FUNC_TRACE();
> >>>> +
> >>>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>>> +		return;
> >>>> +
> >>>
> >>> Hi John,
> >>>
> >>> I am not sure about these updates. Agree that these functions should
> >>> know process type, but all others PMDs don't do this.
> >
> > I looked at mlx5 and it does something similar with its mlx5_is_secondary()
> in functions that modify fields in its shared private structure.
> 
> Right, mlx5 also has these checks.
> 
> >
> >>>
> >>> Added a few more people for comment, but as far I understand its
> >>> application responsibility to NOT call these functions if it is
> >>> secondary process.
> >>>
> >>> For device init/uninit, that is part of eal_init() and have to be
> >>> called both for primary and secondary process and PMD needs to
> >>> protect it, for other functions application's responsibility.
> >
> > I put the checks into the PMD because I didn't want to trust the app and I
> didn't see checks in the library functions. I assumed that is why it was done in
> mlx5. I was afraid that the secondary may try to write fields in the shared
> structure and cause some silent bad behavior, like if secondary sets the MTU
> then the primary does, then secondary assumes it was different than what it
> is, or something like that.
> 
> The set values are in the shared memory, so a variable set by secondary will
> be visible to primary, via versa. Of course a synchronization required
> between primary and secondary processes.
> 
> Also why secondary shouldn't be allowed to do some work, like start/stop
> the port?
> 
> I believe this should be application level concern, at worst libraries but not
> drivers.

I don't disagree, but I would need to verify that relaxing the checks would not cause catastrophic errors in case the primary and secondary don't synchronize their actions to the PMD. This will take some more testing. The patch I provided with the checks is safe as is and so if you could accept it for 17.08 with the promise that I will work on relaxing the checks in the next release, I would appreciate it.
> 
> >>
> >> Yes for now it is the policy.
> >> But it is a gray area and it could be clearer with my "ownership proposal":
> >> 	http://dpdk.org/ml/archives/dev/2017-September/074656.html
> >> A secondary process could manage the ports it owns.
> >
> > I think the ownership concept would help make DPDK more flexible and
> potentially cleaner. Perhaps ownership checks could be pushed the lib
> functions, like rte_eth_dev_set_mtu(), and remove all such checks in the
> PMD(s).
> >>
> >> Feel free to comment the proposal.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
  2017-09-11 18:58 [dpdk-dev] [PATCH] net/enic: fix multi-process operation John Daley
  2017-09-12 13:53 ` Aaron Conole
  2017-09-18 21:27 ` Ferruh Yigit
@ 2017-09-22 16:50 ` Ferruh Yigit
  2 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2017-09-22 16:50 UTC (permalink / raw)
  To: John Daley; +Cc: dev, stable

On 9/11/2017 7:58 PM, John Daley wrote:
> - Use rte_malloc() instead of malloc() for the per device 'vdev' structure
>   so that it can be shared across processes.
> - Only initialize the device if the process type is RTE_PROC_PRIMARY
> - Only allow the primary process to do queue setup, start/stop, promisc
>   allmulticast, mac add/del, mtu.
> 
> Fixes: fefed3d1e62c ("enic: new driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: John Daley <johndale@cisco.com>

Applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-09-22 16:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-11 18:58 [dpdk-dev] [PATCH] net/enic: fix multi-process operation John Daley
2017-09-12 13:53 ` Aaron Conole
2017-09-18 21:27 ` Ferruh Yigit
2017-09-18 22:25   ` Thomas Monjalon
2017-09-19  5:31     ` John Daley (johndale)
2017-09-19 12:58       ` Ferruh Yigit
2017-09-22 13:02         ` John Daley (johndale)
2017-09-22 16:50 ` Ferruh Yigit

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).