[v1,1/2] net/axgbe: add multi-process support
Checks
Commit Message
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(-)
Comments
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,
On 12/21/2022 2:52 AM, Jesna K E wrote:
> +/* 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
> +}
'txq' is used only for 'RTE_ARCH_X86', that is why it gives "unused
variable" warning for it.
Can you please declare 'txq' within 'RTE_ARCH_X86' macro?
On 12/23/2022 10:44 AM, Ferruh Yigit wrote:
> On 12/21/2022 2:52 AM, Jesna K E wrote:
>> +/* 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;
btw, indentation of this line looks wrong
>> +#endif
>> +}
>
> 'txq' is used only for 'RTE_ARCH_X86', that is why it gives "unused
> variable" warning for it.
> Can you please declare 'txq' within 'RTE_ARCH_X86' macro?
@@ -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
@@ -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;
-
+ }
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;
-
pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
pdata->pci_dev = pci_dev;
@@ -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;
}
@@ -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.
+ */
+void axgbe_set_tx_function(struct rte_eth_dev *dev);
+void axgbe_set_rx_function(struct rte_eth_dev *dev);
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,