From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Jesna K E <jesna.k.e@amd.com>, dev@dpdk.org
Cc: Selwin.Sebastian@amd.com
Subject: Re: [PATCH v1 1/2] net/axgbe: add multi-process support
Date: Wed, 21 Dec 2022 15:37:05 +0000 [thread overview]
Message-ID: <01a50e41-6433-a855-25ea-50b94071411d@amd.com> (raw)
In-Reply-To: <20221221025202.31733-1-jesna.k.e@amd.com>
On 12/21/2022 2:52 AM, Jesna K E wrote:
> Added multi-process support for axgbe PMD
> To achieve multi-process support separate out TX and RX function
> inside the axgbe driver and call that from a secondary process
> when it is attaching to an already-configured NIC
>
> Signed-off-by: Jesna K E <jesna.k.e@amd.com>
> ---
> doc/guides/nics/features/axgbe.ini | 1 +
> drivers/net/axgbe/axgbe_ethdev.c | 65 ++++++++++++++++++++++--------
> drivers/net/axgbe/axgbe_rxtx.c | 11 -----
> drivers/net/axgbe/axgbe_rxtx.h | 7 +++-
> 4 files changed, 55 insertions(+), 29 deletions(-)
>
Overall looks good, do you think does it worth to document this in
release notes (doc/guides/rel_notes/release_23_03.rst)?
> diff --git a/doc/guides/nics/features/axgbe.ini b/doc/guides/nics/features/axgbe.ini
> index 821bb682d4..5e2d6498e5 100644
> --- a/doc/guides/nics/features/axgbe.ini
> +++ b/doc/guides/nics/features/axgbe.ini
> @@ -18,6 +18,7 @@ L3 checksum offload = Y
> L4 checksum offload = Y
> Basic stats = Y
> FW version = Y
> +Multiprocess aware = Y
> Linux = Y
> x86-32 = Y
> x86-64 = Y
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index b071e4e460..4b3d655bd3 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -353,8 +353,6 @@ axgbe_dev_start(struct rte_eth_dev *dev)
> {
> struct axgbe_port *pdata = dev->data->dev_private;
> int ret;
> - struct rte_eth_dev_data *dev_data = dev->data;
> - uint16_t max_pkt_len;
>
> dev->dev_ops = &axgbe_eth_dev_ops;
>
> @@ -388,17 +386,8 @@ axgbe_dev_start(struct rte_eth_dev *dev)
> rte_bit_relaxed_clear32(AXGBE_STOPPED, &pdata->dev_state);
> rte_bit_relaxed_clear32(AXGBE_DOWN, &pdata->dev_state);
>
> - max_pkt_len = dev_data->mtu + RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> - if ((dev_data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_SCATTER) ||
> - max_pkt_len > pdata->rx_buf_size)
> - dev_data->scattered_rx = 1;
> -
> - /* Scatter Rx handling */
> - if (dev_data->scattered_rx)
> - dev->rx_pkt_burst = ð_axgbe_recv_scattered_pkts;
> - else
> - dev->rx_pkt_burst = &axgbe_recv_pkts;
> -
> + axgbe_set_rx_function(dev);
> + axgbe_set_tx_function(dev);
> return 0;
> }
>
> @@ -2145,6 +2134,46 @@ get_pci_rc_devid(void)
> return (uint16_t)device_id;
> }
>
> +/* Takes ethdev as parameter
> + * Used in dev_start by primary process and then
> + * in dev_init by secondary process when attaching to an existing ethdev.
> + */
> +void
> +axgbe_set_tx_function(struct rte_eth_dev *dev)
> +{
> + struct axgbe_port *pdata = dev->data->dev_private;
> + struct axgbe_tx_queue *txq = dev->data->tx_queues[0];
> +
> + if (pdata->multi_segs_tx)
> + dev->tx_pkt_burst = &axgbe_xmit_pkts_seg;
> +#ifdef RTE_ARCH_X86
> + if (!txq->vector_disable &&
> + rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_128)
> + dev->tx_pkt_burst = &axgbe_xmit_pkts_vec;
> +#else
> + dev->tx_pkt_burst = &axgbe_xmit_pkts;
> +#endif
> +}
> +
> +void
> +axgbe_set_rx_function(struct rte_eth_dev *dev)
> +{
> + struct rte_eth_dev_data *dev_data = dev->data;
> + uint16_t max_pkt_len;
> + struct axgbe_port *pdata;
> +
> + pdata = dev->data->dev_private;
> + max_pkt_len = dev_data->mtu + RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> + if ((dev_data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_SCATTER) ||
> + max_pkt_len > pdata->rx_buf_size)
> + dev_data->scattered_rx = 1;
> + /* Scatter Rx handling */
> + if (dev_data->scattered_rx)
> + dev->rx_pkt_burst = ð_axgbe_recv_scattered_pkts;
> + else
> + dev->rx_pkt_burst = &axgbe_recv_pkts;
> +}
> +
> /*
> * It returns 0 on success.
> */
> @@ -2159,17 +2188,20 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
> int ret;
>
> eth_dev->dev_ops = &axgbe_eth_dev_ops;
> -
> eth_dev->rx_descriptor_status = axgbe_dev_rx_descriptor_status;
> eth_dev->tx_descriptor_status = axgbe_dev_tx_descriptor_status;
>
> + eth_dev->tx_pkt_burst = &axgbe_xmit_pkts;
> + eth_dev->rx_pkt_burst = &axgbe_recv_pkts;
> /*
> * For secondary processes, we don't initialise any further as primary
> * has already done this work.
> */
> - if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> + axgbe_set_tx_function(eth_dev);
> + axgbe_set_rx_function(eth_dev);
> return 0;
> -
What do you think to keep empty lines before and after secondary process
related block to group it?
> + }
> eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>
> pdata = eth_dev->data->dev_private;
> @@ -2177,7 +2209,6 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
> rte_bit_relaxed_set32(AXGBE_DOWN, &pdata->dev_state);
> rte_bit_relaxed_set32(AXGBE_STOPPED, &pdata->dev_state);
> pdata->eth_dev = eth_dev;
> -
Please remove noise in the patch
> pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
> pdata->pci_dev = pci_dev;
>
> diff --git a/drivers/net/axgbe/axgbe_rxtx.c b/drivers/net/axgbe/axgbe_rxtx.c
> index 7cff79e030..9b283bd9d0 100644
> --- a/drivers/net/axgbe/axgbe_rxtx.c
> +++ b/drivers/net/axgbe/axgbe_rxtx.c
> @@ -629,17 +629,6 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
> RTE_ETH_TX_OFFLOAD_MULTI_SEGS))
> pdata->multi_segs_tx = true;
>
> - if (pdata->multi_segs_tx)
> - dev->tx_pkt_burst = &axgbe_xmit_pkts_seg;
> - else if (txq->vector_disable ||
> - rte_vect_get_max_simd_bitwidth() < RTE_VECT_SIMD_128)
> - dev->tx_pkt_burst = &axgbe_xmit_pkts;
> - else
> -#ifdef RTE_ARCH_X86
> - dev->tx_pkt_burst = &axgbe_xmit_pkts_vec;
> -#else
> - dev->tx_pkt_burst = &axgbe_xmit_pkts;
> -#endif
>
> return 0;
> }
> diff --git a/drivers/net/axgbe/axgbe_rxtx.h b/drivers/net/axgbe/axgbe_rxtx.h
> index eeef908ceb..d5660f5c4b 100644
> --- a/drivers/net/axgbe/axgbe_rxtx.h
> +++ b/drivers/net/axgbe/axgbe_rxtx.h
> @@ -158,7 +158,12 @@ struct axgbe_tx_queue {
> * RX/TX function prototypes
> */
>
> -
> +/* Takes an ethdev and sets up the tx function to be used based on
> + * the queue parameters. Used in dev_start by primary process and then
> + * in dev_init by secondary process when attaching to an existing ethdev.
Is the comment only for Tx, or for burst functions? Since comment refers
only to Tx.
> + */
> +void axgbe_set_tx_function(struct rte_eth_dev *dev);
> +void axgbe_set_rx_function(struct rte_eth_dev *dev);
You can put an empty line here to highlight comment is for these two
functions.
> void axgbe_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t queue_idx);
> int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
> uint16_t nb_tx_desc, unsigned int socket_id,
next prev parent reply other threads:[~2022-12-21 15:37 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-21 2:52 Jesna K E
2022-12-21 2:52 ` [PATCH v1 2/2] net/axgbe: move offloads to Rx/Tx queue setup Jesna K E
2022-12-21 16:27 ` Ferruh Yigit
2022-12-21 15:37 ` Ferruh Yigit [this message]
2022-12-23 10:44 ` [PATCH v1 1/2] net/axgbe: add multi-process support Ferruh Yigit
2022-12-23 10:47 ` Ferruh Yigit
-- strict thread matches above, loose matches on Subject: below --
2023-01-04 7:42 Jesna K E
2022-12-21 2:35 Jesna K E
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=01a50e41-6433-a855-25ea-50b94071411d@amd.com \
--to=ferruh.yigit@amd.com \
--cc=Selwin.Sebastian@amd.com \
--cc=dev@dpdk.org \
--cc=jesna.k.e@amd.com \
/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).