diff mbox series

[v2,1/3] net/ark: support secondary process

Message ID 20230217215923.2561685-1-ed.czeck@atomicrules.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [v2,1/3] net/ark: support secondary process |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Ed Czeck Feb. 17, 2023, 9:59 p.m. UTC 
  From: John Miller <john.miller@atomicrules.com>

disable device configuration for secondary processes

Signed-off-by: John Miller <john.miller@atomicrules.com>
---
v2:
* Use standard logging
---
 drivers/net/ark/ark_ethdev.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Ferruh Yigit Feb. 20, 2023, 2:17 p.m. UTC | #1 
On 2/17/2023 9:59 PM, Ed Czeck wrote:
> From: John Miller <john.miller@atomicrules.com>
> 
> disable device configuration for secondary processes
> 
> Signed-off-by: John Miller <john.miller@atomicrules.com>
> ---
> v2:
> * Use standard logging
> ---
>  drivers/net/ark/ark_ethdev.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
> index b2995427c8..d237e80cf4 100644
> --- a/drivers/net/ark/ark_ethdev.c
> +++ b/drivers/net/ark/ark_ethdev.c
> @@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	struct rte_eth_dev *eth_dev;
>  	int ret;
>  
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> +		ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
> +
>  	eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct ark_adapter));
>  
>  	if (eth_dev == NULL)
> @@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>  		    0xcafef00d, ark->sysctrl.t32[4], __func__);
>  
>  	/* We are a single function multi-port device. */
> -	ret = ark_config_device(dev);
> -	if (ret)
> -		return -1;
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		ret = ark_config_device(dev);
> +		if (ret)
> +			return -1;
> +	}


Hi Ed,

As far as I can see both primary and secondary process continues to run
after this point, and below there are a few places that updates
'eth_dev->data'.

'eth_dev->data' is shared between primary and secondaries, so each
secondary will be overwriting the shared data.
Better usage is shared data only updated by primary process and
secondary processes use available values.
But 'eth_dev' is process specific and all primary and shared processes
must set fields of this struct.

You may need to re-order calls in function to make secondary quit after
'eth_dev' fields updated and before 'eth_dev->data' updated, to make
sure secondaries don't update shared data.

>  
>  	dev->dev_ops = &ark_eth_dev_ops;
>  	dev->rx_queue_count = eth_ark_dev_rx_queue_count;
Ed Czeck Feb. 20, 2023, 10:04 p.m. UTC | #2 
Hi ferruh,
We have limited support for secondary processes.  This patch simply avoids
corrupting the FPGA state if a secondary process attaches.
Improved support for secondary processes is on our list, but we need a
strong customer driver for this feature.
An update patch is following soon.
Thanks for the review.
Ed.

On Mon, Feb 20, 2023 at 9:17 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 2/17/2023 9:59 PM, Ed Czeck wrote:
> > From: John Miller <john.miller@atomicrules.com>
> >
> > disable device configuration for secondary processes
> >
> > Signed-off-by: John Miller <john.miller@atomicrules.com>
> > ---
> > v2:
> > * Use standard logging
> > ---
> >  drivers/net/ark/ark_ethdev.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
> > index b2995427c8..d237e80cf4 100644
> > --- a/drivers/net/ark/ark_ethdev.c
> > +++ b/drivers/net/ark/ark_ethdev.c
> > @@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> >       struct rte_eth_dev *eth_dev;
> >       int ret;
> >
> > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> > +             ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
> > +
> >       eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct
> ark_adapter));
> >
> >       if (eth_dev == NULL)
> > @@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
> >                   0xcafef00d, ark->sysctrl.t32[4], __func__);
> >
> >       /* We are a single function multi-port device. */
> > -     ret = ark_config_device(dev);
> > -     if (ret)
> > -             return -1;
> > +     if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +             ret = ark_config_device(dev);
> > +             if (ret)
> > +                     return -1;
> > +     }
>
>
> Hi Ed,
>
> As far as I can see both primary and secondary process continues to run
> after this point, and below there are a few places that updates
> 'eth_dev->data'.
>
> 'eth_dev->data' is shared between primary and secondaries, so each
> secondary will be overwriting the shared data.
> Better usage is shared data only updated by primary process and
> secondary processes use available values.
> But 'eth_dev' is process specific and all primary and shared processes
> must set fields of this struct.
>
> You may need to re-order calls in function to make secondary quit after
> 'eth_dev' fields updated and before 'eth_dev->data' updated, to make
> sure secondaries don't update shared data.
>
> >
> >       dev->dev_ops = &ark_eth_dev_ops;
> >       dev->rx_queue_count = eth_ark_dev_rx_queue_count;
>
>
Ferruh Yigit Feb. 20, 2023, 11:13 p.m. UTC | #3 
On 2/20/2023 10:04 PM, Ed Czeck wrote:
> Hi ferruh,
> We have limited support for secondary processes.  This patch simply
> avoids corrupting the FPGA state if a secondary process attaches.
> Improved support for secondary processes is on our list, but we need a
> strong customer driver for this feature.
> An update patch is following soon.

Hi Ed,

I see new version just updates commit log, mentioning this is minimal
secondary process support, but current implementation is wrong, it is
not about minimal support or full support.

Like just below secondary process, there is following code:

``dev->data->mac_addrs = rte_zmalloc()``

so you are allocating memory and set it to exact same pointer for
primary and each secondaries. This is probably leaking memory and
secondaries overwriting existing mac config by pointing it to new memory
location.

And there are more `dev->data->xxx` modifications in the function, which
is cause unexpected result.


As mentioned in the previous review, you need to find proper location to
return from function for secondary processes.

Please try to fix the implementation, instead of trying to push it with
known errors.


> Thanks for the review.
> Ed.
> 
> On Mon, Feb 20, 2023 at 9:17 AM Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:ferruh.yigit@amd.com>> wrote:
> 
>     On 2/17/2023 9:59 PM, Ed Czeck wrote:
>     > From: John Miller <john.miller@atomicrules.com
>     <mailto:john.miller@atomicrules.com>>
>     >
>     > disable device configuration for secondary processes
>     >
>     > Signed-off-by: John Miller <john.miller@atomicrules.com
>     <mailto:john.miller@atomicrules.com>>
>     > ---
>     > v2:
>     > * Use standard logging
>     > ---
>     >  drivers/net/ark/ark_ethdev.c | 11 ++++++++---
>     >  1 file changed, 8 insertions(+), 3 deletions(-)
>     >
>     > diff --git a/drivers/net/ark/ark_ethdev.c
>     b/drivers/net/ark/ark_ethdev.c
>     > index b2995427c8..d237e80cf4 100644
>     > --- a/drivers/net/ark/ark_ethdev.c
>     > +++ b/drivers/net/ark/ark_ethdev.c
>     > @@ -147,6 +147,9 @@ eth_ark_pci_probe(struct rte_pci_driver
>     *pci_drv __rte_unused,
>     >       struct rte_eth_dev *eth_dev;
>     >       int ret;
>     > 
>     > +     if (rte_eal_process_type() == RTE_PROC_SECONDARY)
>     > +             ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
>     > +
>     >       eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct
>     ark_adapter));
>     > 
>     >       if (eth_dev == NULL)
>     > @@ -385,9 +388,11 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>     >                   0xcafef00d, ark->sysctrl.t32[4], __func__);
>     > 
>     >       /* We are a single function multi-port device. */
>     > -     ret = ark_config_device(dev);
>     > -     if (ret)
>     > -             return -1;
>     > +     if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>     > +             ret = ark_config_device(dev);
>     > +             if (ret)
>     > +                     return -1;
>     > +     }
> 
> 
>     Hi Ed,
> 
>     As far as I can see both primary and secondary process continues to run
>     after this point, and below there are a few places that updates
>     'eth_dev->data'.
> 
>     'eth_dev->data' is shared between primary and secondaries, so each
>     secondary will be overwriting the shared data.
>     Better usage is shared data only updated by primary process and
>     secondary processes use available values.
>     But 'eth_dev' is process specific and all primary and shared processes
>     must set fields of this struct.
> 
>     You may need to re-order calls in function to make secondary quit after
>     'eth_dev' fields updated and before 'eth_dev->data' updated, to make
>     sure secondaries don't update shared data.
> 
>     > 
>     >       dev->dev_ops = &ark_eth_dev_ops;
>     >       dev->rx_queue_count = eth_ark_dev_rx_queue_count;
>
diff mbox series

Patch

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index b2995427c8..d237e80cf4 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -147,6 +147,9 @@  eth_ark_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	struct rte_eth_dev *eth_dev;
 	int ret;
 
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+		ARK_PMD_LOG(DEBUG, "ARK probed by secondary process\n");
+
 	eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct ark_adapter));
 
 	if (eth_dev == NULL)
@@ -385,9 +388,11 @@  eth_ark_dev_init(struct rte_eth_dev *dev)
 		    0xcafef00d, ark->sysctrl.t32[4], __func__);
 
 	/* We are a single function multi-port device. */
-	ret = ark_config_device(dev);
-	if (ret)
-		return -1;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = ark_config_device(dev);
+		if (ret)
+			return -1;
+	}
 
 	dev->dev_ops = &ark_eth_dev_ops;
 	dev->rx_queue_count = eth_ark_dev_rx_queue_count;