Re: [PATCH net] i40e: Enforce software interrupt during busy-poll exit

From: Michal Schmidt
Date: Wed Mar 13 2024 - 09:47:41 EST

On Wed, Mar 13, 2024 at 1:55 PM Ivan Vecera <ivecera@xxxxxxxxxx> wrote:
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index 9b701615c7c6..4d2b05de6c63 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -908,6 +908,7 @@ struct i40e_q_vector {
> struct rcu_head rcu; /* to avoid race with update stats on free */
> char name[I40E_INT_NAME_STR_LEN];
> bool arm_wb_state;
> + bool in_busy_poll;
> int irq_num; /* IRQ assigned to this q_vector */
> } ____cacheline_internodealigned_in_smp;
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 89a3401d20ab..1ea6d06b0acc 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3915,6 +3915,12 @@ static void i40e_vsi_configure_msix(struct i40e_vsi *vsi)
> q_vector->tx.target_itr >> 1);
> q_vector->tx.current_itr = q_vector->tx.target_itr;
>
> + /* Set ITR for software interrupts triggered after exiting
> + * busy-loop polling.
> + */
> + wr32(hw, I40E_PFINT_ITRN(I40E_SW_ITR, vector - 1),
> + I40E_ITR_20K);
> +
> wr32(hw, I40E_PFINT_RATEN(vector - 1),
> i40e_intrl_usec_to_reg(vsi->int_rate_limit));
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_register.h b/drivers/net/ethernet/intel/i40e/i40e_register.h
> index 14ab642cafdb..baa6bb68bcf8 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_register.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_register.h
> @@ -335,6 +335,8 @@
> #define I40E_PFINT_DYN_CTLN_INTERVAL_SHIFT 5
> #define I40E_PFINT_DYN_CTLN_SW_ITR_INDX_ENA_SHIFT 24
> #define I40E_PFINT_DYN_CTLN_SW_ITR_INDX_ENA_MASK I40E_MASK(0x1, I40E_PFINT_DYN_CTLN_SW_ITR_INDX_ENA_SHIFT)
> +#define I40E_PFINT_DYN_CTLN_SW_ITR_INDX_SHIFT 25
> +#define I40E_PFINT_DYN_CTLN_SW_ITR_INDX_MASK I40E_MASK(0x3, I40E_PFINT_DYN_CTLN_SW_ITR_INDX_SHIFT)
> #define I40E_PFINT_ICR0 0x00038780 /* Reset: CORER */
> #define I40E_PFINT_ICR0_INTEVENT_SHIFT 0
> #define I40E_PFINT_ICR0_INTEVENT_MASK I40E_MASK(0x1, I40E_PFINT_ICR0_INTEVENT_SHIFT)
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 0d7177083708..356c3140adf3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2658,8 +2658,22 @@ static inline u32 i40e_buildreg_itr(const int type, u16 itr)
> return val;
> }
>
> -/* a small macro to shorten up some long lines */
> -#define INTREG I40E_PFINT_DYN_CTLN
> +static inline u32 i40e_buildreg_swint(int type)
> +{
> + u32 val;
> +
> + /* 1. Enable the interrupt
> + * 2. Do not modify any ITR interval
> + * 3. Trigger a SW interrupt specified by type
> + */
> + val = I40E_PFINT_DYN_CTLN_INTENA_MASK |
> + I40E_PFINT_DYN_CTLN_ITR_INDX_MASK | /* set noitr */
> + I40E_PFINT_DYN_CTLN_SWINT_TRIG_MASK |
> + I40E_PFINT_DYN_CTLN_SW_ITR_INDX_ENA_MASK |
> + FIELD_PREP(I40E_PFINT_DYN_CTLN_SW_ITR_INDX_MASK, type);
> +
> + return val;
> +}

This function is called only from one place and with a constant
argument. Does it really need to be a function, as opposed to a
constant? Or are you going to add more callers soon?

>
> /* The act of updating the ITR will cause it to immediately trigger. In order
> * to prevent this from throwing off adaptive update statistics we defer the
> @@ -2702,8 +2716,8 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
> */
> if (q_vector->rx.target_itr < q_vector->rx.current_itr) {
> /* Rx ITR needs to be reduced, this is highest priority */
> - intval = i40e_buildreg_itr(I40E_RX_ITR,
> - q_vector->rx.target_itr);
> + wr32(hw, I40E_PFINT_ITRN(I40E_RX_ITR, q_vector->reg_idx),
> + q_vector->rx.target_itr >> 1);
> q_vector->rx.current_itr = q_vector->rx.target_itr;
> q_vector->itr_countdown = ITR_COUNTDOWN_START;
> } else if ((q_vector->tx.target_itr < q_vector->tx.current_itr) ||
> @@ -2712,25 +2726,33 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
> /* Tx ITR needs to be reduced, this is second priority
> * Tx ITR needs to be increased more than Rx, fourth priority
> */
> - intval = i40e_buildreg_itr(I40E_TX_ITR,
> - q_vector->tx.target_itr);
> + wr32(hw, I40E_PFINT_ITRN(I40E_TX_ITR, q_vector->reg_idx),
> + q_vector->tx.target_itr >> 1);
> q_vector->tx.current_itr = q_vector->tx.target_itr;
> q_vector->itr_countdown = ITR_COUNTDOWN_START;
> } else if (q_vector->rx.current_itr != q_vector->rx.target_itr) {
> /* Rx ITR needs to be increased, third priority */
> - intval = i40e_buildreg_itr(I40E_RX_ITR,
> - q_vector->rx.target_itr);
> + wr32(hw, I40E_PFINT_ITRN(I40E_RX_ITR, q_vector->reg_idx),
> + q_vector->rx.target_itr >> 1);
> q_vector->rx.current_itr = q_vector->rx.target_itr;
> q_vector->itr_countdown = ITR_COUNTDOWN_START;
> } else {
> /* No ITR update, lowest priority */
> - intval = i40e_buildreg_itr(I40E_ITR_NONE, 0);
> if (q_vector->itr_countdown)
> q_vector->itr_countdown--;
> }
>
> - if (!test_bit(__I40E_VSI_DOWN, vsi->state))
> - wr32(hw, INTREG(q_vector->reg_idx), intval);
> + /* Do not enable interrupt if VSI is down */
> + if (test_bit(__I40E_VSI_DOWN, vsi->state))
> + return;
> +
> + if (!q_vector->in_busy_poll) {
> + intval = i40e_buildreg_itr(I40E_ITR_NONE, 0);
> + } else {
> + q_vector->in_busy_poll = false;
> + intval = i40e_buildreg_swint(I40E_SW_ITR);
> + }
> + wr32(hw, I40E_PFINT_DYN_CTLN(q_vector->reg_idx), intval);
> }
>
> /**
> @@ -2845,6 +2867,8 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
> */
> if (likely(napi_complete_done(napi, work_done)))
> i40e_update_enable_itr(vsi, q_vector);
> + else
> + q_vector->in_busy_poll = true;
>
> return min(work_done, budget - 1);
> }
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> index abf15067eb5d..2cdc7de6301c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> @@ -68,6 +68,7 @@ enum i40e_dyn_idx {
> /* these are indexes into ITRN registers */
> #define I40E_RX_ITR I40E_IDX_ITR0
> #define I40E_TX_ITR I40E_IDX_ITR1
> +#define I40E_SW_ITR I40E_IDX_ITR2
>
> /* Supported RSS offloads */
> #define I40E_DEFAULT_RSS_HENA ( \
> --
> 2.43.0
>