| Message ID | 20231212145546.396273-1-michal.kubiak@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] idpf: enable WB_ON_ITR | expand |
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);
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
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
> -----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
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 >
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
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
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
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 > >
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
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);