From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Ed Czeck <ed.czeck@atomicrules.com>
Cc: dev@dpdk.org, stephen@networkplumber.org,
John Miller <john.miller@atomicrules.com>,
Shepard Siegel <shepard.siegel@atomicrules.com>,
Anatoly Burakov <anatoly.burakov@intel.com>
Subject: Re: [PATCH v2 1/3] net/ark: support secondary process
Date: Mon, 20 Feb 2023 23:13:14 +0000 [thread overview]
Message-ID: <9af27b72-0216-7860-e52a-9b984bda3037@amd.com> (raw)
In-Reply-To: <CALZ3GuhUFh0d8tD0gKoEjN5cUMLCX5Apo0ocvfQK2rCg8jHnog@mail.gmail.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;
>
next prev parent reply other threads:[~2023-02-20 23:13 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-17 16:00 [PATCH " Ed Czeck
2023-02-17 16:00 ` [PATCH 2/3] net/ark: support for single function with multiple port Ed Czeck
2023-02-17 16:00 ` [PATCH 3/3] net/ark: resize data field to match fpga access Ed Czeck
2023-02-17 16:00 ` [PATCH] " Ed Czeck
2023-02-17 16:57 ` Ferruh Yigit
2023-02-17 21:59 ` [PATCH v2 1/3] net/ark: support secondary process Ed Czeck
2023-02-17 21:59 ` [PATCH v2 2/3] net/ark: support for single function with multiple port Ed Czeck
2023-02-20 14:54 ` Ferruh Yigit
2023-02-20 22:09 ` Ed Czeck
2023-02-20 23:21 ` Ferruh Yigit
2023-02-17 21:59 ` [PATCH v2 3/3] net/ark: resize data field to match fpga access Ed Czeck
2023-02-20 14:17 ` [PATCH v2 1/3] net/ark: support secondary process Ferruh Yigit
2023-02-20 22:04 ` Ed Czeck
2023-02-20 23:13 ` Ferruh Yigit [this message]
2023-02-20 22:11 ` [PATCH v3 1/2] net/ark: limited support for secondary processes Ed Czeck
2023-02-20 22:11 ` [PATCH v3 2/2] net/ark: support for single function with multiple port Ed Czeck
2023-02-21 16:30 ` [PATCH v4] " Ed Czeck
2023-02-23 14:25 ` Ferruh Yigit
2023-05-16 14:27 ` Ferruh Yigit
2023-05-16 15:33 ` [PATCH 1/3] net/ark: support secondary process Burakov, Anatoly
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9af27b72-0216-7860-e52a-9b984bda3037@amd.com \
--to=ferruh.yigit@amd.com \
--cc=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=ed.czeck@atomicrules.com \
--cc=john.miller@atomicrules.com \
--cc=shepard.siegel@atomicrules.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).