On Mon, Dec 12, 2022 at 2:53 PM David Marchand <[email protected]> wrote: > On Tue, Dec 7, 2021 at 10:02 AM <[email protected]> wrote: > > > > From: Dapeng Yu <[email protected]>
I got a bounce on his mail address, so adding people who submitted/reviewed later fixes for this change. Please guys, have a look. > > > > Currently max frame size is set at queue level, which makes the values > > of the following counters wrong when a jumbo frame is received. > > > > The expected value: > > rx_good_bytes: 0 > > rx_errors: 1 > > rx_oversize_errors: 1 > > > > The actual value: > > rx_good_bytes: 1626 > > rx_errors: 0 > > rx_oversize_errors: 0 > > > > This patch enables setting max frame size at port level, and makes the > > values above right. > > > > Cc: [email protected] > > > > Signed-off-by: Dapeng Yu <[email protected]> > > I am looking again at this patch that triggered regressions in stable > branches, when used with OVS. > > > > --- > > drivers/net/i40e/i40e_ethdev.c | 41 ++++++++++++++++++++++++++++------ > > 1 file changed, 34 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > > index c0bfff43ee..ef9b2b2414 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -386,6 +386,7 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev > > *dev, > > struct rte_ether_addr *mac_addr); > > > > static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); > > +static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size); > > > > static int i40e_ethertype_filter_convert( > > const struct rte_eth_ethertype_filter *input, > > @@ -1709,11 +1710,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void > > *init_params __rte_unused) > > */ > > i40e_add_tx_flow_control_drop_filter(pf); > > > > - /* Set the max frame size to 0x2600 by default, > > - * in case other drivers changed the default value. > > - */ > > - i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, > > NULL); > > - > > If I understand correctly, the max framesize (9728) was always applied > in dev_init, before this change. > > > > /* initialize RSS rule list */ > > TAILQ_INIT(&pf->rss_config_list); > > > > @@ -2364,6 +2360,7 @@ i40e_dev_start(struct rte_eth_dev *dev) > > uint32_t intr_vector = 0; > > struct i40e_vsi *vsi; > > uint16_t nb_rxq, nb_txq; > > + uint16_t max_frame_size; > > > > hw->adapter_stopped = 0; > > > > @@ -2502,6 +2499,9 @@ i40e_dev_start(struct rte_eth_dev *dev) > > "please call hierarchy_commit() " > > "before starting the port"); > > > > + max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD; > > + i40e_set_mac_max_frame(dev, max_frame_size); > > + > > And now this change tries to configure, at dev_start step, the max > frame size that the application actually requested. > Which seems ok, afaiu. > > > > return I40E_SUCCESS; > > > > tx_err: > > @@ -2848,6 +2848,9 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev) > > return i40e_phy_conf_link(hw, abilities, speed, false); > > } > > > > +#define CHECK_INTERVAL 100 /* 100ms */ > > +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > > + > > static __rte_always_inline void > > update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) > > { > > @@ -2914,8 +2917,6 @@ static __rte_always_inline void > > update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link, > > bool enable_lse, int wait_to_complete) > > { > > -#define CHECK_INTERVAL 100 /* 100ms */ > > -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > > uint32_t rep_cnt = MAX_REPEAT_TIME; > > struct i40e_link_status link_status; > > int status; > > @@ -6719,6 +6720,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev) > > if (!ret) > > rte_eth_dev_callback_process(dev, > > RTE_ETH_EVENT_INTR_LSC, NULL); > > + > > break; > > default: > > PMD_DRV_LOG(DEBUG, "Request %u is not supported > > yet", > > @@ -12103,6 +12105,31 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf) > > return ret; > > } > > > > +static void > > +i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) > > +{ > > + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > + uint32_t rep_cnt = MAX_REPEAT_TIME; > > + struct rte_eth_link link; > > + enum i40e_status_code status; > > + > > + do { > > + update_link_reg(hw, &link); > > + if (link.link_status) > > + break; > > + > > + rte_delay_ms(CHECK_INTERVAL); > > + } while (--rep_cnt); > > + > > + if (link.link_status) { > > + status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, > > NULL); > > + if (status != I40E_SUCCESS) > > + PMD_DRV_LOG(ERR, "Failed to set max frame size at > > port level"); > > + } else { > > + PMD_DRV_LOG(ERR, "Set max frame size at port level not > > applicable on link down"); > > + } > > But here, applying the configuration when the link is up only, looks > very wrong to me. > > If the link is down, then this configuration will never get applied. > And what is worse, the application has no indication of it since no > error code is propagated. > > So the application thinks the port started, but it will never get "big" > packets. > > > Like for example: > - unplug cable, > - start testpmd and configure mtu to 9000, > # dpdk-testpmd -l 0,2,3 --in-memory -a 0000:05:00.1 -- -i --disable-link-check > ... > Configuring Port 0 (socket 0) > i40e_set_mac_max_frame(): Set max frame size at port level not > applicable on link down > Port 0: 3C:FD:FE:A0:A4:C5 > Done > > testpmd> port stop 0 > Stopping ports... > Done > > testpmd> port config mtu 0 9000 > > testpmd> port start 0 > i40e_set_mac_max_frame(): Set max frame size at port level not > applicable on link down > Port 0: 3C:FD:FE:A0:A4:C5 > Done > > - plug back cable, and send "big" packets, like for example ping with > -s 8000 from other side of the cable, > - no "big" packet is received on the dpdk port, and its rx_errors > xstats keeps increasing, > > > Looking at the original change, why did you not ignore link status state? > I would just move "i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, > TRUE, false, 0, NULL);" in dev_start. > > -- David Marchand
