[PATCH iwl-net] idpf: enable WB_ON_ITR

[PATCH iwl-net] idpf: enable WB_ON_ITR

[Michal Kubiak]

From: Joshua Hay <joshua.a.hay@intel.com>

Tell hardware to writeback completed descriptors even when interrupts
are disabled. Otherwise, descriptors might not be written back until
the hardware can flush a full cacheline of descriptors. This can cause
unnecessary delays when traffic is light (or even trigger Tx queue
timeout).

Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
Co-developed-by: Michal Kubiak <michal.kubiak@intel.com>
Signed-off-by: Michal Kubiak <michal.kubiak@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_dev.c    |  2 ++
 .../ethernet/intel/idpf/idpf_singleq_txrx.c   |  6 ++++-
 drivers/net/ethernet/intel/idpf/idpf_txrx.c   |  7 +++++-
 drivers/net/ethernet/intel/idpf/idpf_txrx.h   | 23 +++++++++++++++++++
 drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |  2 ++
 5 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_dev.c
index 34ad1ac46b78..2c6776086130 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_dev.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_dev.c
@@ -96,8 +96,10 @@ static int idpf_intr_reg_init(struct idpf_vport *vport)
 		intr->dyn_ctl = idpf_get_reg_addr(adapter,
 						  reg_vals[vec_id].dyn_ctl_reg);
 		intr->dyn_ctl_intena_m = PF_GLINT_DYN_CTL_INTENA_M;
+		intr->dyn_ctl_intena_msk_m = PF_GLINT_DYN_CTL_INTENA_MSK_M;
 		intr->dyn_ctl_itridx_s = PF_GLINT_DYN_CTL_ITR_INDX_S;
 		intr->dyn_ctl_intrvl_s = PF_GLINT_DYN_CTL_INTERVAL_S;
+		intr->dyn_ctl_wb_on_itr_m = PF_GLINT_DYN_CTL_WB_ON_ITR_M;
 
 		spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
 					       IDPF_PF_ITR_IDX_SPACING);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
index 81288a17da2a..8e1478b7d86c 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
@@ -1168,8 +1168,10 @@ int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget)
 						    &work_done);
 
 	/* If work not completed, return budget and polling will return */
-	if (!clean_complete)
+	if (!clean_complete) {
+		idpf_vport_intr_set_wb_on_itr(q_vector);
 		return budget;
+	}
 
 	work_done = min_t(int, work_done, budget - 1);
 
@@ -1178,6 +1180,8 @@ int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget)
 	 */
 	if (likely(napi_complete_done(napi, work_done)))
 		idpf_vport_intr_update_itr_ena_irq(q_vector);
+	else
+		idpf_vport_intr_set_wb_on_itr(q_vector);
 
 	return work_done;
 }
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index 1646ff3877ba..b496566ee2aa 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3639,6 +3639,7 @@ void idpf_vport_intr_update_itr_ena_irq(struct idpf_q_vector *q_vector)
 					      IDPF_NO_ITR_UPDATE_IDX, 0);
 
 	writel(intval, q_vector->intr_reg.dyn_ctl);
+	q_vector->wb_on_itr = false;
 }
 
 /**
@@ -3930,8 +3931,10 @@ static int idpf_vport_splitq_napi_poll(struct napi_struct *napi, int budget)
 	clean_complete &= idpf_tx_splitq_clean_all(q_vector, budget, &work_done);
 
 	/* If work not completed, return budget and polling will return */
-	if (!clean_complete)
+	if (!clean_complete) {
+		idpf_vport_intr_set_wb_on_itr(q_vector);
 		return budget;
+	}
 
 	work_done = min_t(int, work_done, budget - 1);
 
@@ -3940,6 +3943,8 @@ static int idpf_vport_splitq_napi_poll(struct napi_struct *napi, int budget)
 	 */
 	if (likely(napi_complete_done(napi, work_done)))
 		idpf_vport_intr_update_itr_ena_irq(q_vector);
+	else
+		idpf_vport_intr_set_wb_on_itr(q_vector);
 
 	/* Switch to poll mode in the tear-down path after sending disable
 	 * queues virtchnl message, as the interrupts will be disabled after
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
index df76493faa75..50761c2d9f3b 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
@@ -495,9 +495,11 @@ struct idpf_vec_regs {
 struct idpf_intr_reg {
 	void __iomem *dyn_ctl;
 	u32 dyn_ctl_intena_m;
+	u32 dyn_ctl_intena_msk_m;
 	u32 dyn_ctl_itridx_s;
 	u32 dyn_ctl_itridx_m;
 	u32 dyn_ctl_intrvl_s;
+	u32 dyn_ctl_wb_on_itr_m;
 	void __iomem *rx_itr;
 	void __iomem *tx_itr;
 	void __iomem *icr_ena;
@@ -534,6 +536,7 @@ struct idpf_q_vector {
 	struct napi_struct napi;
 	u16 v_idx;
 	struct idpf_intr_reg intr_reg;
+	bool wb_on_itr;
 
 	u16 num_txq;
 	struct idpf_queue **tx;
@@ -973,6 +976,26 @@ static inline void idpf_rx_sync_for_cpu(struct idpf_rx_buf *rx_buf, u32 len)
 				      page_pool_get_dma_dir(pp));
 }
 
+/**
+ * idpf_vport_intr_set_wb_on_itr - enable descriptor writeback on disabled interrupts
+ * @q_vector: pointer to queue vector struct
+ */
+static inline void idpf_vport_intr_set_wb_on_itr(struct idpf_q_vector *q_vector)
+{
+	struct idpf_intr_reg *reg;
+
+	if (q_vector->wb_on_itr)
+		return;
+
+	reg = &q_vector->intr_reg;
+
+	writel(reg->dyn_ctl_wb_on_itr_m | reg->dyn_ctl_intena_msk_m |
+	       IDPF_NO_ITR_UPDATE_IDX << reg->dyn_ctl_itridx_s,
+	       reg->dyn_ctl);
+
+	q_vector->wb_on_itr = true;
+}
+
 int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget);
 void idpf_vport_init_num_qs(struct idpf_vport *vport,
 			    struct virtchnl2_create_vport *vport_msg);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
index 8ade4e3a9fe1..f5b0a0666636 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
@@ -96,7 +96,9 @@ static int idpf_vf_intr_reg_init(struct idpf_vport *vport)
 		intr->dyn_ctl = idpf_get_reg_addr(adapter,
 						  reg_vals[vec_id].dyn_ctl_reg);
 		intr->dyn_ctl_intena_m = VF_INT_DYN_CTLN_INTENA_M;
+		intr->dyn_ctl_intena_msk_m = VF_INT_DYN_CTLN_INTENA_MSK_M;
 		intr->dyn_ctl_itridx_s = VF_INT_DYN_CTLN_ITR_INDX_S;
+		intr->dyn_ctl_wb_on_itr_m = VF_INT_DYN_CTLN_WB_ON_ITR_M;
 
 		spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
 					       IDPF_VF_ITR_IDX_SPACING);
-- 
2.33.1

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Paul Menzel]

Dear Michal, dear Joshua,


Thank you for your patch.

On 12/12/23 15:55, Michal Kubiak wrote:
> From: Joshua Hay <joshua.a.hay@intel.com>
> 
> Tell hardware to writeback completed descriptors even when interrupts

Should you resend, the verb is spelled with a space: write back.

> are disabled. Otherwise, descriptors might not be written back until
> the hardware can flush a full cacheline of descriptors. This can cause
> unnecessary delays when traffic is light (or even trigger Tx queue
> timeout).

How can the problem be reproduced and the patch be verified?


Kind regards,

Paul


> Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
> Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> Co-developed-by: Michal Kubiak <michal.kubiak@intel.com>
> Signed-off-by: Michal Kubiak <michal.kubiak@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_dev.c    |  2 ++
>  .../ethernet/intel/idpf/idpf_singleq_txrx.c   |  6 ++++-
>  drivers/net/ethernet/intel/idpf/idpf_txrx.c   |  7 +++++-
>  drivers/net/ethernet/intel/idpf/idpf_txrx.h   | 23 +++++++++++++++++++
>  drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |  2 ++
>  5 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_dev.c
> index 34ad1ac46b78..2c6776086130 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_dev.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_dev.c
> @@ -96,8 +96,10 @@ static int idpf_intr_reg_init(struct idpf_vport *vport)
>  		intr->dyn_ctl = idpf_get_reg_addr(adapter,
>  						  reg_vals[vec_id].dyn_ctl_reg);
>  		intr->dyn_ctl_intena_m = PF_GLINT_DYN_CTL_INTENA_M;
> +		intr->dyn_ctl_intena_msk_m = PF_GLINT_DYN_CTL_INTENA_MSK_M;
>  		intr->dyn_ctl_itridx_s = PF_GLINT_DYN_CTL_ITR_INDX_S;
>  		intr->dyn_ctl_intrvl_s = PF_GLINT_DYN_CTL_INTERVAL_S;
> +		intr->dyn_ctl_wb_on_itr_m = PF_GLINT_DYN_CTL_WB_ON_ITR_M;
>  
>  		spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
>  					       IDPF_PF_ITR_IDX_SPACING);
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> index 81288a17da2a..8e1478b7d86c 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> @@ -1168,8 +1168,10 @@ int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget)
>  						    &work_done);
>  
>  	/* If work not completed, return budget and polling will return */
> -	if (!clean_complete)
> +	if (!clean_complete) {
> +		idpf_vport_intr_set_wb_on_itr(q_vector);
>  		return budget;
> +	}
>  
>  	work_done = min_t(int, work_done, budget - 1);
>  
> @@ -1178,6 +1180,8 @@ int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget)
>  	 */
>  	if (likely(napi_complete_done(napi, work_done)))
>  		idpf_vport_intr_update_itr_ena_irq(q_vector);
> +	else
> +		idpf_vport_intr_set_wb_on_itr(q_vector);
>  
>  	return work_done;
>  }
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 1646ff3877ba..b496566ee2aa 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> @@ -3639,6 +3639,7 @@ void idpf_vport_intr_update_itr_ena_irq(struct idpf_q_vector *q_vector)
>  					      IDPF_NO_ITR_UPDATE_IDX, 0);
>  
>  	writel(intval, q_vector->intr_reg.dyn_ctl);
> +	q_vector->wb_on_itr = false;
>  }
>  
>  /**
> @@ -3930,8 +3931,10 @@ static int idpf_vport_splitq_napi_poll(struct napi_struct *napi, int budget)
>  	clean_complete &= idpf_tx_splitq_clean_all(q_vector, budget, &work_done);
>  
>  	/* If work not completed, return budget and polling will return */
> -	if (!clean_complete)
> +	if (!clean_complete) {
> +		idpf_vport_intr_set_wb_on_itr(q_vector);
>  		return budget;
> +	}
>  
>  	work_done = min_t(int, work_done, budget - 1);
>  
> @@ -3940,6 +3943,8 @@ static int idpf_vport_splitq_napi_poll(struct napi_struct *napi, int budget)
>  	 */
>  	if (likely(napi_complete_done(napi, work_done)))
>  		idpf_vport_intr_update_itr_ena_irq(q_vector);
> +	else
> +		idpf_vport_intr_set_wb_on_itr(q_vector);
>  
>  	/* Switch to poll mode in the tear-down path after sending disable
>  	 * queues virtchnl message, as the interrupts will be disabled after
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> index df76493faa75..50761c2d9f3b 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> @@ -495,9 +495,11 @@ struct idpf_vec_regs {
>  struct idpf_intr_reg {
>  	void __iomem *dyn_ctl;
>  	u32 dyn_ctl_intena_m;
> +	u32 dyn_ctl_intena_msk_m;
>  	u32 dyn_ctl_itridx_s;
>  	u32 dyn_ctl_itridx_m;
>  	u32 dyn_ctl_intrvl_s;
> +	u32 dyn_ctl_wb_on_itr_m;
>  	void __iomem *rx_itr;
>  	void __iomem *tx_itr;
>  	void __iomem *icr_ena;
> @@ -534,6 +536,7 @@ struct idpf_q_vector {
>  	struct napi_struct napi;
>  	u16 v_idx;
>  	struct idpf_intr_reg intr_reg;
> +	bool wb_on_itr;
>  
>  	u16 num_txq;
>  	struct idpf_queue **tx;
> @@ -973,6 +976,26 @@ static inline void idpf_rx_sync_for_cpu(struct idpf_rx_buf *rx_buf, u32 len)
>  				      page_pool_get_dma_dir(pp));
>  }
>  
> +/**
> + * idpf_vport_intr_set_wb_on_itr - enable descriptor writeback on disabled interrupts
> + * @q_vector: pointer to queue vector struct
> + */
> +static inline void idpf_vport_intr_set_wb_on_itr(struct idpf_q_vector *q_vector)
> +{
> +	struct idpf_intr_reg *reg;
> +
> +	if (q_vector->wb_on_itr)
> +		return;
> +
> +	reg = &q_vector->intr_reg;
> +
> +	writel(reg->dyn_ctl_wb_on_itr_m | reg->dyn_ctl_intena_msk_m |
> +	       IDPF_NO_ITR_UPDATE_IDX << reg->dyn_ctl_itridx_s,
> +	       reg->dyn_ctl);
> +
> +	q_vector->wb_on_itr = true;
> +}
> +
>  int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget);
>  void idpf_vport_init_num_qs(struct idpf_vport *vport,
>  			    struct virtchnl2_create_vport *vport_msg);
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
> index 8ade4e3a9fe1..f5b0a0666636 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
> @@ -96,7 +96,9 @@ static int idpf_vf_intr_reg_init(struct idpf_vport *vport)
>  		intr->dyn_ctl = idpf_get_reg_addr(adapter,
>  						  reg_vals[vec_id].dyn_ctl_reg);
>  		intr->dyn_ctl_intena_m = VF_INT_DYN_CTLN_INTENA_M;
> +		intr->dyn_ctl_intena_msk_m = VF_INT_DYN_CTLN_INTENA_MSK_M;
>  		intr->dyn_ctl_itridx_s = VF_INT_DYN_CTLN_ITR_INDX_S;
> +		intr->dyn_ctl_wb_on_itr_m = VF_INT_DYN_CTLN_WB_ON_ITR_M;
>  
>  		spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
>  					       IDPF_VF_ITR_IDX_SPACING);

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Michal Kubiak]

On Tue, Dec 12, 2023 at 05:50:55PM +0100, Paul Menzel wrote:
> Dear Michal, dear Joshua,
> 
> 
> Thank you for your patch.
> 
> On 12/12/23 15:55, Michal Kubiak wrote:
> > From: Joshua Hay <joshua.a.hay@intel.com>
> > 
> > Tell hardware to writeback completed descriptors even when interrupts
> 
> Should you resend, the verb is spelled with a space: write back.

Sure, I will fix it.

> 
> > are disabled. Otherwise, descriptors might not be written back until
> > the hardware can flush a full cacheline of descriptors. This can cause
> > unnecessary delays when traffic is light (or even trigger Tx queue
> > timeout).
> 
> How can the problem be reproduced and the patch be verified?
> 
> 
> Kind regards,
> 
> Paul
> 
> 

Hi Paul,

To be honest, I have noticed the problem during the implementation of
AF_XDP feature for IDPF driver. In my scenario, I had 2 Tx queues:
 - regular LAN Tx queue
 - and XDP Tx queue
added to the same q_vector attached to the same NAPI, so those 2 Tx
queues were handled in the same NAPI poll loop.
Then, when I started a huge Tx zero-copy trafic using AF_XDP (on the XDP
queue), and, at the same time, tried to xmit a few packets using the second
(non-XDP) queue (e.g. with scapy), I was getting the Tx timeout on that regular
LAN Tx queue.
That is why I decided to upstream this fix. With disabled writebacks,
there is no chance to get the completion descriptor for the queue where
the traffic is much lighter.

I have never tried to reproduce the scenario described by Joshua
in his original patch ("unnecessary delays when traffic is light").

Thanks,
Michal

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Michal Kubiak]

On Wed, Dec 13, 2023 at 02:23:19PM +0100, Michal Kubiak wrote:
> On Tue, Dec 12, 2023 at 05:50:55PM +0100, Paul Menzel wrote:
> > Dear Michal, dear Joshua,
> > 
> > 
> > Thank you for your patch.
> > 
> > On 12/12/23 15:55, Michal Kubiak wrote:
> > > From: Joshua Hay <joshua.a.hay@intel.com>
> > > 
> > > Tell hardware to writeback completed descriptors even when interrupts
> > 
> > Should you resend, the verb is spelled with a space: write back.
> 
> Sure, I will fix it.

Hi Tony,

Could you please add a space ("writeback" -> "write back") when taking the patch
to your tree?

Thanks,
Michal

RE: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Nguyen, Anthony L]

> -----Original Message-----
> From: Kubiak, Michal <michal.kubiak@intel.com>
> Sent: Wednesday, December 13, 2023 5:51 AM
> 
> On Wed, Dec 13, 2023 at 02:23:19PM +0100, Michal Kubiak wrote:
> > On Tue, Dec 12, 2023 at 05:50:55PM +0100, Paul Menzel wrote:
> > > Dear Michal, dear Joshua,
> > >
> > >
> > > Thank you for your patch.
> > >
> > > On 12/12/23 15:55, Michal Kubiak wrote:
> > > > From: Joshua Hay <joshua.a.hay@intel.com>
> > > >
> > > > Tell hardware to writeback completed descriptors even when
> > > > interrupts
> > >
> > > Should you resend, the verb is spelled with a space: write back.
> >
> > Sure, I will fix it.
> 
> Hi Tony,
> 
> Could you please add a space ("writeback" -> "write back") when taking the
> patch to your tree?

Yep, I can do that.

Thanks,
Tony

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Tony Nguyen]

On 12/13/2023 2:22 PM, Nguyen, Anthony L wrote:
> 
> 
>> -----Original Message-----
>> From: Kubiak, Michal <michal.kubiak@intel.com>
>> Sent: Wednesday, December 13, 2023 5:51 AM
>>
>> On Wed, Dec 13, 2023 at 02:23:19PM +0100, Michal Kubiak wrote:
>>> On Tue, Dec 12, 2023 at 05:50:55PM +0100, Paul Menzel wrote:
>>>> Dear Michal, dear Joshua,
>>>>
>>>>
>>>> Thank you for your patch.
>>>>
>>>> On 12/12/23 15:55, Michal Kubiak wrote:
>>>>> From: Joshua Hay <joshua.a.hay@intel.com>
>>>>>
>>>>> Tell hardware to writeback completed descriptors even when
>>>>> interrupts
>>>>
>>>> Should you resend, the verb is spelled with a space: write back.
>>>
>>> Sure, I will fix it.
>>
>> Hi Tony,
>>
>> Could you please add a space ("writeback" -> "write back") when taking the
>> patch to your tree?
> 
> Yep, I can do that.

Actually, looks like you missed updating kdocs

drivers/net/ethernet/intel/idpf/idpf_txrx.h:508: warning: Function 
parameter or member 'dyn_ctl_intena_msk_m' not described in 'idpf_intr_reg'
drivers/net/ethernet/intel/idpf/idpf_txrx.h:508: warning: Function 
parameter or member 'dyn_ctl_wb_on_itr_m' not described in 'idpf_intr_reg'
drivers/net/ethernet/intel/idpf/idpf_txrx.h:561: warning: Function 
parameter or member 'wb_on_itr' not described in 'idpf_q_vector'

> Thanks,
> Tony
> 

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Michal Kubiak]

On Wed, Dec 13, 2023 at 03:06:09PM -0800, Tony Nguyen wrote:
> 
> 
> On 12/13/2023 2:22 PM, Nguyen, Anthony L wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Kubiak, Michal <michal.kubiak@intel.com>
> > > Sent: Wednesday, December 13, 2023 5:51 AM
> > > 
> > > On Wed, Dec 13, 2023 at 02:23:19PM +0100, Michal Kubiak wrote:
> > > > On Tue, Dec 12, 2023 at 05:50:55PM +0100, Paul Menzel wrote:
> > > > > Dear Michal, dear Joshua,
> > > > > 
> > > > > 
> > > > > Thank you for your patch.
> > > > > 
> > > > > On 12/12/23 15:55, Michal Kubiak wrote:
> > > > > > From: Joshua Hay <joshua.a.hay@intel.com>
> > > > > > 
> > > > > > Tell hardware to writeback completed descriptors even when
> > > > > > interrupts
> > > > > 
> > > > > Should you resend, the verb is spelled with a space: write back.
> > > > 
> > > > Sure, I will fix it.
> > > 
> > > Hi Tony,
> > > 
> > > Could you please add a space ("writeback" -> "write back") when taking the
> > > patch to your tree?
> > 
> > Yep, I can do that.
> 
> Actually, looks like you missed updating kdocs
> 
> drivers/net/ethernet/intel/idpf/idpf_txrx.h:508: warning: Function parameter
> or member 'dyn_ctl_intena_msk_m' not described in 'idpf_intr_reg'
> drivers/net/ethernet/intel/idpf/idpf_txrx.h:508: warning: Function parameter
> or member 'dyn_ctl_wb_on_itr_m' not described in 'idpf_intr_reg'
> drivers/net/ethernet/intel/idpf/idpf_txrx.h:561: warning: Function parameter
> or member 'wb_on_itr' not described in 'idpf_q_vector'
> 
> > Thanks,
> > Tony

Thank you, Tony, for reporting that!
So I will fix kdocs and missing space in v2.

Thanks,
Michal

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Paul Menzel]

Lieber Michal,


Am 13.12.23 um 14:23 schrieb Michal Kubiak:
> On Tue, Dec 12, 2023 at 05:50:55PM +0100, Paul Menzel wrote:

>> On 12/12/23 15:55, Michal Kubiak wrote:
>>> From: Joshua Hay <joshua.a.hay@intel.com>
>>>
>>> Tell hardware to writeback completed descriptors even when interrupts
>>
>> Should you resend, the verb is spelled with a space: write back.
> 
> Sure, I will fix it.

Thanks.

>>> are disabled. Otherwise, descriptors might not be written back until
>>> the hardware can flush a full cacheline of descriptors. This can cause
>>> unnecessary delays when traffic is light (or even trigger Tx queue
>>> timeout).
>>
>> How can the problem be reproduced and the patch be verified?

[…]

> To be honest, I have noticed the problem during the implementation of
> AF_XDP feature for IDPF driver. In my scenario, I had 2 Tx queues:
>   - regular LAN Tx queue
>   - and XDP Tx queue
> added to the same q_vector attached to the same NAPI, so those 2 Tx
> queues were handled in the same NAPI poll loop.
> Then, when I started a huge Tx zero-copy trafic using AF_XDP (on the XDP
> queue), and, at the same time, tried to xmit a few packets using the second
> (non-XDP) queue (e.g. with scapy), I was getting the Tx timeout on that regular
> LAN Tx queue.
> That is why I decided to upstream this fix. With disabled writebacks,
> there is no chance to get the completion descriptor for the queue where
> the traffic is much lighter.
> 
> I have never tried to reproduce the scenario described by Joshua
> in his original patch ("unnecessary delays when traffic is light").

Understood. Maybe you could add a summary of the above to the commit 
message or just copy and paste. I guess, it’s better than nothing. And 
thank you for upstreaming this.


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR

[Michal Kubiak]

On Thu, Dec 14, 2023 at 01:59:57PM +0100, Paul Menzel wrote:
> Lieber Michal,
> 
> 
> Am 13.12.23 um 14:23 schrieb Michal Kubiak:
> > On Tue, Dec 12, 2023 at 05:50:55PM +0100, Paul Menzel wrote:
> 
> > > On 12/12/23 15:55, Michal Kubiak wrote:
> > > > From: Joshua Hay <joshua.a.hay@intel.com>
> > > > 
> > > > Tell hardware to writeback completed descriptors even when interrupts
> > > 
> > > Should you resend, the verb is spelled with a space: write back.
> > 
> > Sure, I will fix it.
> 
> Thanks.

Will be fixed in v2.

> 
> > > > are disabled. Otherwise, descriptors might not be written back until
> > > > the hardware can flush a full cacheline of descriptors. This can cause
> > > > unnecessary delays when traffic is light (or even trigger Tx queue
> > > > timeout).
> > > 
> > > How can the problem be reproduced and the patch be verified?
> 
> […]
> 
> > To be honest, I have noticed the problem during the implementation of
> > AF_XDP feature for IDPF driver. In my scenario, I had 2 Tx queues:
> >   - regular LAN Tx queue
> >   - and XDP Tx queue
> > added to the same q_vector attached to the same NAPI, so those 2 Tx
> > queues were handled in the same NAPI poll loop.
> > Then, when I started a huge Tx zero-copy trafic using AF_XDP (on the XDP
> > queue), and, at the same time, tried to xmit a few packets using the second
> > (non-XDP) queue (e.g. with scapy), I was getting the Tx timeout on that regular
> > LAN Tx queue.
> > That is why I decided to upstream this fix. With disabled writebacks,
> > there is no chance to get the completion descriptor for the queue where
> > the traffic is much lighter.
> > 
> > I have never tried to reproduce the scenario described by Joshua
> > in his original patch ("unnecessary delays when traffic is light").
> 
> Understood. Maybe you could add a summary of the above to the commit message
> or just copy and paste. I guess, it’s better than nothing. And thank you for
> upstreaming this.
> 
> 
> Kind regards,
> 
> Paul

Right. I have to add some kdocs, so I will also extend that commit message in v2.

Thanks,
Michal

Re: [PATCH iwl-net] idpf: enable WB_ON_ITR

[Brett Creeley]

On 12/12/2023 6:55 AM, Michal Kubiak wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: Joshua Hay <joshua.a.hay@intel.com>
> 
> Tell hardware to writeback completed descriptors even when interrupts
> are disabled. Otherwise, descriptors might not be written back until
> the hardware can flush a full cacheline of descriptors. This can cause
> unnecessary delays when traffic is light (or even trigger Tx queue
> timeout).
> 
> Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
> Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> Co-developed-by: Michal Kubiak <michal.kubiak@intel.com>
> Signed-off-by: Michal Kubiak <michal.kubiak@intel.com>
> ---
>   drivers/net/ethernet/intel/idpf/idpf_dev.c    |  2 ++
>   .../ethernet/intel/idpf/idpf_singleq_txrx.c   |  6 ++++-
>   drivers/net/ethernet/intel/idpf/idpf_txrx.c   |  7 +++++-
>   drivers/net/ethernet/intel/idpf/idpf_txrx.h   | 23 +++++++++++++++++++
>   drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |  2 ++
>   5 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_dev.c
> index 34ad1ac46b78..2c6776086130 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_dev.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_dev.c
> @@ -96,8 +96,10 @@ static int idpf_intr_reg_init(struct idpf_vport *vport)
>                  intr->dyn_ctl = idpf_get_reg_addr(adapter,
>                                                    reg_vals[vec_id].dyn_ctl_reg);
>                  intr->dyn_ctl_intena_m = PF_GLINT_DYN_CTL_INTENA_M;
> +               intr->dyn_ctl_intena_msk_m = PF_GLINT_DYN_CTL_INTENA_MSK_M;
>                  intr->dyn_ctl_itridx_s = PF_GLINT_DYN_CTL_ITR_INDX_S;
>                  intr->dyn_ctl_intrvl_s = PF_GLINT_DYN_CTL_INTERVAL_S;
> +               intr->dyn_ctl_wb_on_itr_m = PF_GLINT_DYN_CTL_WB_ON_ITR_M;
> 
>                  spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
>                                                 IDPF_PF_ITR_IDX_SPACING);
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> index 81288a17da2a..8e1478b7d86c 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> @@ -1168,8 +1168,10 @@ int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget)
>                                                      &work_done);
> 
>          /* If work not completed, return budget and polling will return */
> -       if (!clean_complete)
> +       if (!clean_complete) {
> +               idpf_vport_intr_set_wb_on_itr(q_vector);
>                  return budget;
> +       }
> 
>          work_done = min_t(int, work_done, budget - 1);
> 
> @@ -1178,6 +1180,8 @@ int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget)
>           */
>          if (likely(napi_complete_done(napi, work_done)))
>                  idpf_vport_intr_update_itr_ena_irq(q_vector);
> +       else
> +               idpf_vport_intr_set_wb_on_itr(q_vector);
> 
>          return work_done;
>   }
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 1646ff3877ba..b496566ee2aa 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> @@ -3639,6 +3639,7 @@ void idpf_vport_intr_update_itr_ena_irq(struct idpf_q_vector *q_vector)
>                                                IDPF_NO_ITR_UPDATE_IDX, 0);
> 
>          writel(intval, q_vector->intr_reg.dyn_ctl);
> +       q_vector->wb_on_itr = false;
>   }
> 
>   /**
> @@ -3930,8 +3931,10 @@ static int idpf_vport_splitq_napi_poll(struct napi_struct *napi, int budget)
>          clean_complete &= idpf_tx_splitq_clean_all(q_vector, budget, &work_done);
> 
>          /* If work not completed, return budget and polling will return */
> -       if (!clean_complete)
> +       if (!clean_complete) {
> +               idpf_vport_intr_set_wb_on_itr(q_vector);
>                  return budget;
> +       }
> 
>          work_done = min_t(int, work_done, budget - 1);
> 
> @@ -3940,6 +3943,8 @@ static int idpf_vport_splitq_napi_poll(struct napi_struct *napi, int budget)
>           */
>          if (likely(napi_complete_done(napi, work_done)))
>                  idpf_vport_intr_update_itr_ena_irq(q_vector);
> +       else
> +               idpf_vport_intr_set_wb_on_itr(q_vector);
> 
>          /* Switch to poll mode in the tear-down path after sending disable
>           * queues virtchnl message, as the interrupts will be disabled after
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> index df76493faa75..50761c2d9f3b 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> @@ -495,9 +495,11 @@ struct idpf_vec_regs {
>   struct idpf_intr_reg {
>          void __iomem *dyn_ctl;
>          u32 dyn_ctl_intena_m;
> +       u32 dyn_ctl_intena_msk_m;
>          u32 dyn_ctl_itridx_s;
>          u32 dyn_ctl_itridx_m;
>          u32 dyn_ctl_intrvl_s;
> +       u32 dyn_ctl_wb_on_itr_m;
>          void __iomem *rx_itr;
>          void __iomem *tx_itr;
>          void __iomem *icr_ena;
> @@ -534,6 +536,7 @@ struct idpf_q_vector {
>          struct napi_struct napi;
>          u16 v_idx;
>          struct idpf_intr_reg intr_reg;
> +       bool wb_on_itr;

Not sure if this was considered, but it might be best to put this before 
intr_reg so it's on the same cacheline as intr_reg.

> 
>          u16 num_txq;
>          struct idpf_queue **tx;
> @@ -973,6 +976,26 @@ static inline void idpf_rx_sync_for_cpu(struct idpf_rx_buf *rx_buf, u32 len)
>                                        page_pool_get_dma_dir(pp));
>   }
> 
> +/**
> + * idpf_vport_intr_set_wb_on_itr - enable descriptor writeback on disabled interrupts
> + * @q_vector: pointer to queue vector struct
> + */
> +static inline void idpf_vport_intr_set_wb_on_itr(struct idpf_q_vector *q_vector)
> +{
> +       struct idpf_intr_reg *reg;
> +
> +       if (q_vector->wb_on_itr)
> +               return;
> +
> +       reg = &q_vector->intr_reg;
> +
> +       writel(reg->dyn_ctl_wb_on_itr_m | reg->dyn_ctl_intena_msk_m |
> +              IDPF_NO_ITR_UPDATE_IDX << reg->dyn_ctl_itridx_s,
> +              reg->dyn_ctl);
> +
> +       q_vector->wb_on_itr = true;
> +}
> +
>   int idpf_vport_singleq_napi_poll(struct napi_struct *napi, int budget);
>   void idpf_vport_init_num_qs(struct idpf_vport *vport,
>                              struct virtchnl2_create_vport *vport_msg);
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
> index 8ade4e3a9fe1..f5b0a0666636 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
> @@ -96,7 +96,9 @@ static int idpf_vf_intr_reg_init(struct idpf_vport *vport)
>                  intr->dyn_ctl = idpf_get_reg_addr(adapter,
>                                                    reg_vals[vec_id].dyn_ctl_reg);
>                  intr->dyn_ctl_intena_m = VF_INT_DYN_CTLN_INTENA_M;
> +               intr->dyn_ctl_intena_msk_m = VF_INT_DYN_CTLN_INTENA_MSK_M;
>                  intr->dyn_ctl_itridx_s = VF_INT_DYN_CTLN_ITR_INDX_S;
> +               intr->dyn_ctl_wb_on_itr_m = VF_INT_DYN_CTLN_WB_ON_ITR_M;
> 
>                  spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
>                                                 IDPF_VF_ITR_IDX_SPACING);
> --
> 2.33.1
> 
> 

Re: [PATCH iwl-net] idpf: enable WB_ON_ITR

[Michal Kubiak]

On Fri, Dec 15, 2023 at 10:01:36AM -0800, Brett Creeley wrote:
> 

[...]

> 
> > 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> > index df76493faa75..50761c2d9f3b 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> > @@ -495,9 +495,11 @@ struct idpf_vec_regs {
> >   struct idpf_intr_reg {
> >          void __iomem *dyn_ctl;
> >          u32 dyn_ctl_intena_m;
> > +       u32 dyn_ctl_intena_msk_m;
> >          u32 dyn_ctl_itridx_s;
> >          u32 dyn_ctl_itridx_m;
> >          u32 dyn_ctl_intrvl_s;
> > +       u32 dyn_ctl_wb_on_itr_m;
> >          void __iomem *rx_itr;
> >          void __iomem *tx_itr;
> >          void __iomem *icr_ena;
> > @@ -534,6 +536,7 @@ struct idpf_q_vector {
> >          struct napi_struct napi;
> >          u16 v_idx;
> >          struct idpf_intr_reg intr_reg;
> > +       bool wb_on_itr;
> 
> Not sure if this was considered, but it might be best to put this before
> intr_reg so it's on the same cacheline as intr_reg.
> 

Good point! It wasn't considered before.
I have just confirmed with pahole that it is worth putting that flag
before the register structure in terms of cacheline.

Thank you for your suggestion. I will reorder this.

Thanks,
Michal