* [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-12 14:55 ` Michal Kubiak
0 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-12 14:55 UTC (permalink / raw)
To: intel-wired-lan
Cc: aleksander.lobakin, larysa.zaremba, alan.brady, joshua.a.hay,
emil.s.tantilov, maciej.fijalkowski, netdev, Przemek Kitszel,
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-12 14:55 ` Michal Kubiak
0 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-12 14:55 UTC (permalink / raw)
To: intel-wired-lan
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
joshua.a.hay, aleksander.lobakin, Michal Kubiak, alan.brady,
Przemek Kitszel
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-12 14:55 ` [Intel-wired-lan] " Michal Kubiak
@ 2023-12-12 16:50 ` Paul Menzel
-1 siblings, 0 replies; 22+ messages in thread
From: Paul Menzel @ 2023-12-12 16:50 UTC (permalink / raw)
To: Michal Kubiak, Joshua Hay
Cc: intel-wired-lan, maciej.fijalkowski, emil.s.tantilov,
larysa.zaremba, netdev, aleksander.lobakin, alan.brady,
Przemek Kitszel
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);
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-12 16:50 ` Paul Menzel
0 siblings, 0 replies; 22+ messages in thread
From: Paul Menzel @ 2023-12-12 16:50 UTC (permalink / raw)
To: Michal Kubiak, Joshua Hay
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
aleksander.lobakin, intel-wired-lan, alan.brady, Przemek Kitszel
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);
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-12 16:50 ` Paul Menzel
@ 2023-12-13 13:23 ` Michal Kubiak
-1 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-13 13:23 UTC (permalink / raw)
To: Paul Menzel
Cc: Joshua Hay, intel-wired-lan, maciej.fijalkowski, emil.s.tantilov,
larysa.zaremba, netdev, aleksander.lobakin, alan.brady,
Przemek Kitszel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-13 13:23 ` Michal Kubiak
0 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-13 13:23 UTC (permalink / raw)
To: Paul Menzel
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
Joshua Hay, aleksander.lobakin, intel-wired-lan, alan.brady,
Przemek Kitszel
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-13 13:23 ` Michal Kubiak
@ 2023-12-13 13:51 ` Michal Kubiak
-1 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-13 13:51 UTC (permalink / raw)
To: Paul Menzel, Tony Nguyen
Cc: Joshua Hay, intel-wired-lan, maciej.fijalkowski, emil.s.tantilov,
larysa.zaremba, netdev, aleksander.lobakin, alan.brady,
Przemek Kitszel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-13 13:51 ` Michal Kubiak
0 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-13 13:51 UTC (permalink / raw)
To: Paul Menzel, Tony Nguyen
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
Joshua Hay, aleksander.lobakin, intel-wired-lan, alan.brady,
Przemek Kitszel
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-13 13:51 ` Michal Kubiak
@ 2023-12-13 22:22 ` Nguyen, Anthony L
-1 siblings, 0 replies; 22+ messages in thread
From: Nguyen, Anthony L @ 2023-12-13 22:22 UTC (permalink / raw)
To: Kubiak, Michal, Paul Menzel
Cc: Fijalkowski, Maciej, Tantilov, Emil S, Zaremba, Larysa, netdev,
Hay, Joshua A, Lobakin, Aleksander, intel-wired-lan, Brady, Alan,
Kitszel, Przemyslaw
> -----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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-13 22:22 ` Nguyen, Anthony L
0 siblings, 0 replies; 22+ messages in thread
From: Nguyen, Anthony L @ 2023-12-13 22:22 UTC (permalink / raw)
To: Kubiak, Michal, Paul Menzel
Cc: Hay, Joshua A, intel-wired-lan, Fijalkowski, Maciej, Tantilov,
Emil S, Zaremba, Larysa, netdev, Lobakin, Aleksander, Brady,
Alan, Kitszel, Przemyslaw
> -----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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-13 22:22 ` Nguyen, Anthony L
@ 2023-12-13 23:06 ` Tony Nguyen
-1 siblings, 0 replies; 22+ messages in thread
From: Tony Nguyen @ 2023-12-13 23:06 UTC (permalink / raw)
To: Kubiak, Michal, Paul Menzel
Cc: Fijalkowski, Maciej, Tantilov, Emil S, Zaremba, Larysa, netdev,
Hay, Joshua A, Lobakin, Aleksander, intel-wired-lan, Brady, Alan,
Kitszel, Przemyslaw
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
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-13 23:06 ` Tony Nguyen
0 siblings, 0 replies; 22+ messages in thread
From: Tony Nguyen @ 2023-12-13 23:06 UTC (permalink / raw)
To: Kubiak, Michal, Paul Menzel
Cc: Hay, Joshua A, intel-wired-lan, Fijalkowski, Maciej, Tantilov,
Emil S, Zaremba, Larysa, netdev, Lobakin, Aleksander, Brady,
Alan, Kitszel, Przemyslaw
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
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-13 13:23 ` Michal Kubiak
@ 2023-12-14 12:59 ` Paul Menzel
-1 siblings, 0 replies; 22+ messages in thread
From: Paul Menzel @ 2023-12-14 12:59 UTC (permalink / raw)
To: Michal Kubiak
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
Joshua Hay, aleksander.lobakin, intel-wired-lan, alan.brady,
Przemek Kitszel
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-14 12:59 ` Paul Menzel
0 siblings, 0 replies; 22+ messages in thread
From: Paul Menzel @ 2023-12-14 12:59 UTC (permalink / raw)
To: Michal Kubiak
Cc: Joshua Hay, intel-wired-lan, maciej.fijalkowski, emil.s.tantilov,
larysa.zaremba, netdev, aleksander.lobakin, alan.brady,
Przemek Kitszel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-13 23:06 ` Tony Nguyen
@ 2023-12-15 17:27 ` Michal Kubiak
-1 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-15 17:27 UTC (permalink / raw)
To: Tony Nguyen
Cc: Paul Menzel, Fijalkowski, Maciej, Tantilov, Emil S, Zaremba,
Larysa, netdev, Hay, Joshua A, Lobakin, Aleksander,
intel-wired-lan, Brady, Alan, Kitszel, Przemyslaw
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-15 17:27 ` Michal Kubiak
0 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-15 17:27 UTC (permalink / raw)
To: Tony Nguyen
Cc: Paul Menzel, Hay, Joshua A, intel-wired-lan, Fijalkowski, Maciej,
Tantilov, Emil S, Zaremba, Larysa, netdev, Lobakin, Aleksander,
Brady, Alan, Kitszel, Przemyslaw
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-14 12:59 ` Paul Menzel
@ 2023-12-15 17:32 ` Michal Kubiak
-1 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-15 17:32 UTC (permalink / raw)
To: Paul Menzel
Cc: Joshua Hay, intel-wired-lan, maciej.fijalkowski, emil.s.tantilov,
larysa.zaremba, netdev, aleksander.lobakin, alan.brady,
Przemek Kitszel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-15 17:32 ` Michal Kubiak
0 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-15 17:32 UTC (permalink / raw)
To: Paul Menzel
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
Joshua Hay, aleksander.lobakin, intel-wired-lan, alan.brady,
Przemek Kitszel
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-12 14:55 ` [Intel-wired-lan] " Michal Kubiak
@ 2023-12-15 18:01 ` Brett Creeley
-1 siblings, 0 replies; 22+ messages in thread
From: Brett Creeley @ 2023-12-15 18:01 UTC (permalink / raw)
To: Michal Kubiak, intel-wired-lan
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
joshua.a.hay, aleksander.lobakin, alan.brady, Przemek Kitszel
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
>
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-15 18:01 ` Brett Creeley
0 siblings, 0 replies; 22+ messages in thread
From: Brett Creeley @ 2023-12-15 18:01 UTC (permalink / raw)
To: Michal Kubiak, intel-wired-lan
Cc: aleksander.lobakin, larysa.zaremba, alan.brady, joshua.a.hay,
emil.s.tantilov, maciej.fijalkowski, netdev, Przemek Kitszel
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
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iwl-net] idpf: enable WB_ON_ITR
2023-12-15 18:01 ` Brett Creeley
@ 2023-12-15 19:04 ` Michal Kubiak
-1 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-15 19:04 UTC (permalink / raw)
To: Brett Creeley
Cc: intel-wired-lan, aleksander.lobakin, larysa.zaremba, alan.brady,
joshua.a.hay, emil.s.tantilov, maciej.fijalkowski, netdev,
Przemek Kitszel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] idpf: enable WB_ON_ITR
@ 2023-12-15 19:04 ` Michal Kubiak
0 siblings, 0 replies; 22+ messages in thread
From: Michal Kubiak @ 2023-12-15 19:04 UTC (permalink / raw)
To: Brett Creeley
Cc: maciej.fijalkowski, emil.s.tantilov, larysa.zaremba, netdev,
joshua.a.hay, aleksander.lobakin, intel-wired-lan, alan.brady,
Przemek Kitszel
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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-12-15 19:06 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-12 14:55 [PATCH iwl-net] idpf: enable WB_ON_ITR Michal Kubiak
2023-12-12 14:55 ` [Intel-wired-lan] " Michal Kubiak
2023-12-12 16:50 ` Paul Menzel
2023-12-12 16:50 ` Paul Menzel
2023-12-13 13:23 ` Michal Kubiak
2023-12-13 13:23 ` Michal Kubiak
2023-12-13 13:51 ` Michal Kubiak
2023-12-13 13:51 ` Michal Kubiak
2023-12-13 22:22 ` Nguyen, Anthony L
2023-12-13 22:22 ` Nguyen, Anthony L
2023-12-13 23:06 ` Tony Nguyen
2023-12-13 23:06 ` Tony Nguyen
2023-12-15 17:27 ` Michal Kubiak
2023-12-15 17:27 ` Michal Kubiak
2023-12-14 12:59 ` Paul Menzel
2023-12-14 12:59 ` Paul Menzel
2023-12-15 17:32 ` Michal Kubiak
2023-12-15 17:32 ` Michal Kubiak
2023-12-15 18:01 ` Brett Creeley
2023-12-15 18:01 ` Brett Creeley
2023-12-15 19:04 ` Michal Kubiak
2023-12-15 19:04 ` [Intel-wired-lan] " Michal Kubiak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.