From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B9A4041CF1; Mon, 20 Feb 2023 23:09:39 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 78CDC43102; Mon, 20 Feb 2023 23:09:39 +0100 (CET) Received: from mail-lj1-f171.google.com (mail-lj1-f171.google.com [209.85.208.171]) by mails.dpdk.org (Postfix) with ESMTP id D0B8840395 for ; Mon, 20 Feb 2023 23:09:37 +0100 (CET) Received: by mail-lj1-f171.google.com with SMTP id j17so2658223ljq.11 for ; Mon, 20 Feb 2023 14:09:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=atomicrules-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=puqnztj1aU+5Vz5ILTbpAXqQj1Mf+Ba0ybmvoP3uifk=; b=4aUtJ5RcT+gZbaQXuSdHJszY7stYxUjR3kjkD/Se9TYO3ihnBlyKPZdNpvVz0DYPG+ axFEDomvMg0LKzG+wRl1mKzg8aFf/sQiBi6F/MbZwVUSpDPMYje//PohARWZilw8nfGv LXjRxW4kYmdeFrGZc0amSMSYmNej7QSW/ZP55ixLJ9udxpThOoC8kEhOjmwO8DOTeozM 4odc0Q6I+9ujI8wehYZ/CWP+S31YeweLsUMzhUYfWF/UyXFp7O9f+ve4lBCBGOqx6cmw 3ga3oRjOAJU5zZCRQubVV1Jc+ppYgKi0E6RhA5b1WY2PwibzoP6+84ZS3BaMLxHZ4c6q CtBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=puqnztj1aU+5Vz5ILTbpAXqQj1Mf+Ba0ybmvoP3uifk=; b=mzv3CWq04DuDnVdWxDZuTTknq3MVizYUARd0qxWAirlWZkpnRreAuB8gOfKvm1OsAE Bd0uUTKTkuqx5O2RqpmnnYnYyYnmABfzXVF4MI8QsYq/06bIV/dgQMJD8SseZ8ANH9rq gV4NK/fsmzgj30h/0FHcdeRv9A2EYIYY4/kgSKeCKgYYbajig4FXecYAAtGJaQuy0Pyu iNb6z5OoKbkv1aOMUySaMH6BOutjSzMSVZlLUOdvyTNE7jo4TAqmfDRE7Oo+5msdn0NW 6XF6IvVO8o5PL/kaGGe4IjfeO30fHIh5N2K6fZEhNbLhXKa/ECqEWPTtPIB2Bf79p29E Ev4A== X-Gm-Message-State: AO0yUKWXuSBlJI0G+8Rota/3FpW1PG4n3AacVAnTuhlq7u9L+bDfM8r+ cmNsX8Klovj2zbnm/8MASzUuX0O9TOSWDHRuIVc8Hw== X-Google-Smtp-Source: AK7set+skvo9LXI6mamxLDJ3r67QpPfwF85AIb6W3NtMIf5gMWz8p0XwFHJ4gV3sYRdqFqAFT2/K+m6P+K9rd90fbx0= X-Received: by 2002:a05:651c:317:b0:293:4e16:89b3 with SMTP id a23-20020a05651c031700b002934e1689b3mr1030221ljp.3.1676930977206; Mon, 20 Feb 2023 14:09:37 -0800 (PST) MIME-Version: 1.0 References: <20230217160039.2487085-1-ed.czeck@atomicrules.com> <20230217215923.2561685-1-ed.czeck@atomicrules.com> <20230217215923.2561685-2-ed.czeck@atomicrules.com> <390a5b75-175d-3c03-a690-b062ff5de43b@amd.com> In-Reply-To: <390a5b75-175d-3c03-a690-b062ff5de43b@amd.com> From: Ed Czeck Date: Mon, 20 Feb 2023 17:09:25 -0500 Message-ID: Subject: Re: [PATCH v2 2/3] net/ark: support for single function with multiple port To: Ferruh Yigit Cc: dev@dpdk.org, stephen@networkplumber.org, Shepard Siegel , John Miller Content-Type: multipart/alternative; boundary="00000000000035a77605f528ebc5" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --00000000000035a77605f528ebc5 Content-Type: text/plain; charset="UTF-8" Hi Ferruh, We had looked at providing this patch as an LTS backport, but there are some ties to FPGA firmware which complicate its application. As this is a uncommon net/ark feature we support users on an as-needed basis. Thanks. Ed. On Mon, Feb 20, 2023 at 9:54 AM Ferruh Yigit wrote: > On 2/17/2023 9:59 PM, Ed Czeck wrote: > > allows the creation of multiple ports from one ark device via > > the use of ark pmd extension, though the splitting of queues > > Hi Ed, > > As far as I can see "single function with multiple port" support was > already there but this commit is fixing queue index usage for it, if > correct can you please update commit log accordingly? (with fixes line > etc..) > This also helps the fix to be backported to LTS versions. > > > btw, how this feature was working until now, when queue ids "0 to > ark_api_num_queues_per_port()" used for each port? > Was it not tested at all, or is there something changed in FW causing > this issue, if so is there any FW version dependency to this change? > > > > > Add unique dev_private data for each port > > > > Please check comment below related to this one. > > > > > Signed-off-by: Ed Czeck > > --- > > drivers/net/ark/ark_ethdev.c | 14 +++++++++++++- > > drivers/net/ark/ark_ethdev_rx.c | 6 +++--- > > drivers/net/ark/ark_ethdev_tx.c | 2 +- > > drivers/net/ark/ark_global.h | 4 ++++ > > 4 files changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c > > index d237e80cf4..96d0c2b0f0 100644 > > --- a/drivers/net/ark/ark_ethdev.c > > +++ b/drivers/net/ark/ark_ethdev.c > > @@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev) > > ark->user_ext.dev_get_port_count(dev, > > ark->user_data[dev->data->port_id]); > > ark->num_ports = port_count; > > + ark->num_queues = ark_api_num_queues_per_port(ark->mpurx.v, > port_count); > > > > for (p = 0; p < port_count; p++) { > > struct rte_eth_dev *eth_dev; > > @@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev) > > } > > > > eth_dev->device = &pci_dev->device; > > - eth_dev->data->dev_private = ark; > > + /* Device requires new dev_private data */ > > + eth_dev->data->dev_private = > > + rte_zmalloc_socket(name, > > + sizeof(struct ark_adapter), > > + RTE_CACHE_LINE_SIZE, > > + rte_socket_id()); > > + > > + memcpy(eth_dev->data->dev_private, ark, > > + sizeof(struct ark_adapter))> + ark = > eth_dev->data->dev_private; > > + ark->qbase = p * ark->num_queues; > > + > > These each are a new eth_dev, so nothing wrong for each to allocate > device private data, but if the only difference in private data is > 'ark->qbase', it is possible to use 'eth_dev->process_private' for it, > which is per eth_dev. It is up to you. > > > Btw, how there are handled in the secondary process? (previous patch) > Since this code just creates new eth_dev, shouldn't secondary process > needs some code to find and re-use them? > > > eth_dev->dev_ops = ark->eth_dev->dev_ops; > > eth_dev->tx_pkt_burst = ark->eth_dev->tx_pkt_burst; > > eth_dev->rx_pkt_burst = ark->eth_dev->rx_pkt_burst; > > diff --git a/drivers/net/ark/ark_ethdev_rx.c > b/drivers/net/ark/ark_ethdev_rx.c > > index cbc0416bc2..38bc69dff4 100644 > > --- a/drivers/net/ark/ark_ethdev_rx.c > > +++ b/drivers/net/ark/ark_ethdev_rx.c > > @@ -68,7 +68,7 @@ struct ark_rx_queue { > > static int > > eth_ark_rx_hw_setup(struct rte_eth_dev *dev, > > struct ark_rx_queue *queue, > > - uint16_t rx_queue_id __rte_unused, uint16_t > rx_queue_idx) > > + uint16_t rx_queue_idx) > > { > > rte_iova_t queue_base; > > rte_iova_t phys_addr_q_base; > > @@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev, > > uint32_t i; > > int status; > > > > - int qidx = queue_idx; > > + int qidx = ark->qbase + queue_idx; > > > > /* We may already be setup, free memory prior to re-allocation */ > > if (dev->data->rx_queues[queue_idx] != NULL) { > > @@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev, > > } > > /* MPU Setup */ > > if (status == 0) > > - status = eth_ark_rx_hw_setup(dev, queue, qidx, queue_idx); > > + status = eth_ark_rx_hw_setup(dev, queue, queue_idx); > > > > if (unlikely(status != 0)) { > > struct rte_mbuf **mbuf; > > diff --git a/drivers/net/ark/ark_ethdev_tx.c > b/drivers/net/ark/ark_ethdev_tx.c > > index 5940a592a2..4792754f19 100644 > > --- a/drivers/net/ark/ark_ethdev_tx.c > > +++ b/drivers/net/ark/ark_ethdev_tx.c > > @@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev, > > struct ark_tx_queue *queue; > > int status; > > > > - int qidx = queue_idx; > > + int qidx = ark->qbase + queue_idx; > > > > if (!rte_is_power_of_2(nb_desc)) { > > ARK_PMD_LOG(ERR, > > diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global.h > > index 71d0b53e03..176fbcda17 100644 > > --- a/drivers/net/ark/ark_global.h > > +++ b/drivers/net/ark/ark_global.h > > @@ -112,7 +112,11 @@ struct ark_adapter { > > ark_pkt_chkr_t pc; > > ark_pkt_dir_t pd; > > > > + /* For single function, multiple ports */ > > int num_ports; > > + uint16_t qbase; > > + uint16_t num_queues; > > > it looks like 'num_queues' only used locally and not needed to be part > of device data, unless there is some more usage planned for future. > > > + > > bool isvf; > > > > /* Packet generator/checker args */ > > --00000000000035a77605f528ebc5 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Ferruh,
We had looked a= t providing this patch as an LTS backport, but there are some ties to FPGA = firmware which complicate its application.=C2=A0 As this is a uncommon net/= ark feature we support users on an as-needed basis.
Thanks.
=
Ed.

On Mon, Feb 20, 2023 at 9:54 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
On 2/17/2023 9:59 PM, = Ed Czeck wrote:
> allows the creation of multiple ports from one ark device via
> the use of ark pmd extension, though the splitting of queues

Hi Ed,

As far as I can see "single function with multiple port" support = was
already there but this commit is fixing queue index usage for it, if
correct can you please update commit log accordingly? (with fixes line
etc..)
This also helps the fix to be backported to LTS versions.


btw, how this feature was working until now, when queue ids "0 to
ark_api_num_queues_per_port()" used for each port?
Was it not tested at all, or is there something changed in FW causing
this issue, if so is there any FW version dependency to this change?

>
> Add unique dev_private data for each port
>

Please check comment below related to this one.

>
> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> ---
>=C2=A0 drivers/net/ark/ark_ethdev.c=C2=A0 =C2=A0 | 14 +++++++++++++- >=C2=A0 drivers/net/ark/ark_ethdev_rx.c |=C2=A0 6 +++---
>=C2=A0 drivers/net/ark/ark_ethdev_tx.c |=C2=A0 2 +-
>=C2=A0 drivers/net/ark/ark_global.h=C2=A0 =C2=A0 |=C2=A0 4 ++++
>=C2=A0 4 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev= .c
> index d237e80cf4..96d0c2b0f0 100644
> --- a/drivers/net/ark/ark_ethdev.c
> +++ b/drivers/net/ark/ark_ethdev.c
> @@ -432,6 +432,7 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0ark->user_ext.dev_get_port_count(dev,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ark->user_data[dev->data-&g= t;port_id]);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0ark->num_ports =3D port_count;
> +=C2=A0 =C2=A0 =C2=A0ark->num_queues =3D ark_api_num_queues_per_por= t(ark->mpurx.v, port_count);
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0for (p =3D 0; p < port_count; p++) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct rte_eth_d= ev *eth_dev;
> @@ -457,7 +458,18 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0eth_dev->devi= ce =3D &pci_dev->device;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0eth_dev->data->= dev_private =3D ark;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Device requires ne= w dev_private data */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0eth_dev->data->= dev_private =3D
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0rte_zmalloc_socket(name,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 size= of(struct ark_adapter),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RTE_= CACHE_LINE_SIZE,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rte_= socket_id());
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memcpy(eth_dev->da= ta->dev_private, ark,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= sizeof(struct ark_adapter))> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= ark =3D eth_dev->data->dev_private;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ark->qbase =3D p *= ark->num_queues;
> +

These each are a new eth_dev, so nothing wrong for each to allocate
device private data, but if the only difference in private data is
'ark->qbase', it is possible to use 'eth_dev->process_pri= vate' for it,
which is per eth_dev. It is up to you.


Btw, how there are handled in the secondary process? (previous patch)
Since this code just creates new eth_dev, shouldn't secondary process needs some code to find and re-use them?

>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0eth_dev->dev_= ops =3D ark->eth_dev->dev_ops;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0eth_dev->tx_p= kt_burst =3D ark->eth_dev->tx_pkt_burst;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0eth_dev->rx_p= kt_burst =3D ark->eth_dev->rx_pkt_burst;
> diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_eth= dev_rx.c
> index cbc0416bc2..38bc69dff4 100644
> --- a/drivers/net/ark/ark_ethdev_rx.c
> +++ b/drivers/net/ark/ark_ethdev_rx.c
> @@ -68,7 +68,7 @@ struct ark_rx_queue {
>=C2=A0 static int
>=C2=A0 eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0st= ruct ark_rx_queue *queue,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_= t rx_queue_id __rte_unused, uint16_t rx_queue_idx)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_= t rx_queue_idx)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0rte_iova_t queue_base;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0rte_iova_t phys_addr_q_base;
> @@ -124,7 +124,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev= ,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32_t i;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int status;
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0int qidx =3D queue_idx;
> +=C2=A0 =C2=A0 =C2=A0int qidx =3D ark->qbase + queue_idx;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* We may already be setup, free memory prio= r to re-allocation */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (dev->data->rx_queues[queue_idx] != =3D NULL) {
> @@ -215,7 +215,7 @@ eth_ark_dev_rx_queue_setup(struct rte_eth_dev *dev= ,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* MPU Setup */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (status =3D=3D 0)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D eth_ark_rx= _hw_setup(dev, queue, qidx, queue_idx);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D eth_ark_rx= _hw_setup(dev, queue, queue_idx);
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (unlikely(status !=3D 0)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct rte_mbuf = **mbuf;
> diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_eth= dev_tx.c
> index 5940a592a2..4792754f19 100644
> --- a/drivers/net/ark/ark_ethdev_tx.c
> +++ b/drivers/net/ark/ark_ethdev_tx.c
> @@ -229,7 +229,7 @@ eth_ark_tx_queue_setup(struct rte_eth_dev *dev, >=C2=A0 =C2=A0 =C2=A0 =C2=A0struct ark_tx_queue *queue;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int status;
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0int qidx =3D queue_idx;
> +=C2=A0 =C2=A0 =C2=A0int qidx =3D ark->qbase + queue_idx;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!rte_is_power_of_2(nb_desc)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ARK_PMD_LOG(ERR,=
> diff --git a/drivers/net/ark/ark_global.h b/drivers/net/ark/ark_global= .h
> index 71d0b53e03..176fbcda17 100644
> --- a/drivers/net/ark/ark_global.h
> +++ b/drivers/net/ark/ark_global.h
> @@ -112,7 +112,11 @@ struct ark_adapter {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0ark_pkt_chkr_t pc;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0ark_pkt_dir_t pd;
>=C2=A0
> +=C2=A0 =C2=A0 =C2=A0/* For single function, multiple ports */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int num_ports;
> +=C2=A0 =C2=A0 =C2=A0uint16_t qbase;
> +=C2=A0 =C2=A0 =C2=A0uint16_t num_queues;


it looks like 'num_queues' only used locally and not needed to be p= art
of device data, unless there is some more usage planned for future.

> +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0bool isvf;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Packet generator/checker args */

--00000000000035a77605f528ebc5--