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....@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,
