Re: [dpdk-dev] [PATCH 3/3] net/bonding: start ethdev prior to setting 8023ad flow

[Matan Azrad <matan@nvidia.com>]

From: Havlík Martin
> Dne 2021-06-23 09:04, Min Hu (Connor) napsal:
> > 在 2021/6/22 17:25, Martin Havlik 写道:
> >> When dedicated queues are enabled, mlx5 PMD fails to install RTE
> >> Flows if the underlying ethdev is not started:
> >> bond_ethdev_8023ad_flow_set(267) - bond_ethdev_8023ad_flow_set:
> port
> >> not started (slave_port=0 queue_id=1)
> >>
> > Why mlx5 PMD doing flow create relys on port started ?
> > I noticed other PMDs did not has that reliance.
> >
> After looking into it, I really can't answer this mlx5 centered question.
> Closest related info we found so far is the 5th point in
> https://doc.dpdk.org/guides/prog_guide/rte_flow.html#caveats
> but it only specifies it's the application's responsibility and that flow rules are
> assumed invalid after port stop/close/restart but doesn't say anything about
> <stop - flow rule create - start> vs <stop - start - flow rule create> where the
> former is the point of failure on mlx5.
> I'm addressing the maintainers for mlx5, who might know a bit more on the
> topic.

From rte_ethdev.h

* Please note that some configuration is not stored between calls to
 * rte_eth_dev_stop()/rte_eth_dev_start(). The following configuration will
 * be retained:
 *
 *     - MTU
 *     - flow control settings
 *     - receive mode configuration (promiscuous mode, all-multicast mode,
 *       hardware checksum mode, RSS/VMDQ settings etc.)
 *     - VLAN filtering configuration
 *     - default MAC address
 *     - MAC addresses supplied to MAC address array
 *     - flow director filtering mode (but not filtering rules)
 *     - NIC queue statistics mappings


Mlx5 assumes flows are allowed to be configured only after rte_eth_dev_start().
Before start \ after stop - no flow is valid anymore.

Matan

> >> Signed-off-by: Martin Havlik <xhavli56@stud.fit.vutbr.cz>
> >> Cc: Jan Viktorin <viktorin@cesnet.cz>
> >> ---
> >>   drivers/net/bonding/rte_eth_bond_pmd.c | 26
> >> ++++++++++++++++++--------
> >>   1 file changed, 18 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> >> b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> index a6755661c..fea3bc537 100644
> >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> @@ -1818,25 +1818,35 @@ slave_configure(struct rte_eth_dev
> >> *bonded_eth_dev,
> >>                      rte_flow_destroy(slave_eth_dev->data->port_id,
> >>
> >>                                      internals-
> >mode4.dedicated_queues.flow[slave_eth_dev->data->port_id],
> >>                                      &flow_error);
> >> +    }
> >>   +  /* Start device */
> >> +    errval = rte_eth_dev_start(slave_eth_dev->data->port_id);
> >> +    if (errval != 0) {
> >> +            RTE_BOND_LOG(ERR, "rte_eth_dev_start: port=%u, err (%d)",
> >> +                            slave_eth_dev->data->port_id, errval);
> >> +            return -1;
> >> +    }
> >> +
> >> +    if (internals->mode == BONDING_MODE_8023AD &&
> >> +                    internals->mode4.dedicated_queues.enabled == 1)
> >> + {
> >>              errval = bond_ethdev_8023ad_flow_set(bonded_eth_dev,
> >>                              slave_eth_dev->data->port_id);
> >>              if (errval != 0) {
> >>                      RTE_BOND_LOG(ERR,
> >>                              "bond_ethdev_8023ad_flow_set: port=%d, err (%d)",
> >>                              slave_eth_dev->data->port_id, errval);
> >> +
> >> +                    errval = rte_eth_dev_stop(slave_eth_dev->data->port_id);
> >> +                    if (errval < 0) {
> >> +                            RTE_BOND_LOG(ERR,
> >> +                                    "rte_eth_dev_stop: port=%d, err (%d)",
> >> +                                    slave_eth_dev->data->port_id, errval);
> >> +                    }
> >>                      return errval;
> >>              }
> >>      }
> >>   -  /* Start device */
> >> -    errval = rte_eth_dev_start(slave_eth_dev->data->port_id);
> >> -    if (errval != 0) {
> >> -            RTE_BOND_LOG(ERR, "rte_eth_dev_start: port=%u, err (%d)",
> >> -                            slave_eth_dev->data->port_id, errval);
> >> -            return -1;
> >> -    }
> >> -
> >>      /* If RSS is enabled for bonding, synchronize RETA */
> >>      if (bonded_eth_dev->data->dev_conf.rxmode.mq_mode &
> >> ETH_MQ_RX_RSS) {
> >>              int i;
> >>