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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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;
>