| Message ID | 1723072626-32221-1-git-send-email-longli@linuxonhyperv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,net] net: mana: Fix doorbell out of order violation and avoid unnecessary doorbell rings | expand |
On Wed, Aug 07, 2024 at 04:17:06PM -0700, longli@linuxonhyperv.com wrote: > From: Long Li <longli@microsoft.com> > > After napi_complete_done() is called when NAPI is polling in the current > process context, another NAPI may be scheduled and start running in > softirq on another CPU and may ring the doorbell before the current CPU > does. When combined with unnecessary rings when there is no need to arm > the CQ, it triggers error paths in the hardware. > > This patch fixes this by calling napi_complete_done() after doorbell > rings. It limits the number of unnecessary rings when there is > no need to arm. MANA hardware specifies that there must be one doorbell > ring every 8 CQ wraparounds. This driver guarantees one doorbell ring as > soon as the number of consumed CQEs exceeds 4 CQ wraparounds. In pratical > workloads, the 4 CQ wraparounds proves to be big enough that it rarely > exceeds this limit before all the napi weight is consumed. > > To implement this, add a per-CQ counter cq->work_done_since_doorbell, > and make sure the CQ is armed as soon as passing 4 wraparounds of the CQ. > > Cc: stable@vger.kernel.org > Fixes: e1b5683ff62e ("net: mana: Move NAPI from EQ to CQ") > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > Signed-off-by: Long Li <longli@microsoft.com> > --- > > change in v2: > Added more details to comments to explain the patch > > drivers/net/ethernet/microsoft/mana/mana_en.c | 24 ++++++++++++------- > include/net/mana/mana.h | 1 + > 2 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c > index d2f07e179e86..f83211f9e737 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > @@ -1788,7 +1788,6 @@ static void mana_poll_rx_cq(struct mana_cq *cq) > static int mana_cq_handler(void *context, struct gdma_queue *gdma_queue) > { > struct mana_cq *cq = context; > - u8 arm_bit; > int w; > > WARN_ON_ONCE(cq->gdma_cq != gdma_queue); > @@ -1799,16 +1798,23 @@ static int mana_cq_handler(void *context, struct gdma_queue *gdma_queue) > mana_poll_tx_cq(cq); > > w = cq->work_done; > - > - if (w < cq->budget && > - napi_complete_done(&cq->napi, w)) { > - arm_bit = SET_ARM_BIT; > - } else { > - arm_bit = 0; > + cq->work_done_since_doorbell += w; > + > + if (w < cq->budget) { > + mana_gd_ring_cq(gdma_queue, SET_ARM_BIT); > + cq->work_done_since_doorbell = 0; > + napi_complete_done(&cq->napi, w); > + } else if (cq->work_done_since_doorbell > > + cq->gdma_cq->queue_size / COMP_ENTRY_SIZE * 4) { should we define a macro for 4? may be 'CQ_WRAPAROUND_LIMIT' > + /* MANA hardware requires at least one doorbell ring every 8 > + * wraparounds of CQ even if there is no need to arm the CQ. > + * This driver rings the doorbell as soon as we have exceeded > + * 4 wraparounds. > + */ > + mana_gd_ring_cq(gdma_queue, 0); > + cq->work_done_since_doorbell = 0; > } > > - mana_gd_ring_cq(gdma_queue, arm_bit); > - > return w; > } > > diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h > index 6439fd8b437b..7caa334f4888 100644 > --- a/include/net/mana/mana.h > +++ b/include/net/mana/mana.h > @@ -275,6 +275,7 @@ struct mana_cq { > /* NAPI data */ > struct napi_struct napi; > int work_done; > + int work_done_since_doorbell; > int budget; > }; > > -- > 2.17.1
On Wed, 7 Aug 2024 16:17:06 -0700 longli@linuxonhyperv.com wrote: > Cc: stable@vger.kernel.org > Fixes: e1b5683ff62e ("net: mana: Move NAPI from EQ to CQ") > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> no empty lines between trailers please
> > + if (w < cq->budget) { > > + mana_gd_ring_cq(gdma_queue, SET_ARM_BIT); > > + cq->work_done_since_doorbell = 0; > > + napi_complete_done(&cq->napi, w); > > + } else if (cq->work_done_since_doorbell > > > + cq->gdma_cq->queue_size / COMP_ENTRY_SIZE * 4) { > > should we define a macro for 4? may be 'CQ_WRAPAROUND_LIMIT' I prefer to leaving the code as is. This is the only place it's used, and there is a comment explaining why this value is chosen. > > > + /* MANA hardware requires at least one doorbell ring every 8 > > + * wraparounds of CQ even if there is no need to arm the CQ. > > + * This driver rings the doorbell as soon as we have exceeded > > + * 4 wraparounds. > > + */ > > + mana_gd_ring_cq(gdma_queue, 0); > > + cq->work_done_since_doorbell = 0; > > 2.17.1
> Subject: Re: [PATCH v2 net] net: mana: Fix doorbell out of order violation and > avoid unnecessary doorbell rings > > On Wed, 7 Aug 2024 16:17:06 -0700 longli@linuxonhyperv.com wrote: > > Cc: stable@vger.kernel.org > > Fixes: e1b5683ff62e ("net: mana: Move NAPI from EQ to CQ") > > > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > > no empty lines between trailers please I'm sending v3 to fix this.
On Thu, 8 Aug 2024 15:33:55 +0000 Long Li wrote: > > no empty lines between trailers please > > I'm sending v3 to fix this. I hope you don't mean you're sending it _now_, given: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
On Wed, Aug 07, 2024 at 04:17:06PM -0700, longli@linuxonhyperv.com wrote: > From: Long Li <longli@microsoft.com> > > After napi_complete_done() is called when NAPI is polling in the current > process context, another NAPI may be scheduled and start running in > softirq on another CPU and may ring the doorbell before the current CPU > does. When combined with unnecessary rings when there is no need to arm > the CQ, it triggers error paths in the hardware. > > This patch fixes this by calling napi_complete_done() after doorbell > rings. It limits the number of unnecessary rings when there is > no need to arm. MANA hardware specifies that there must be one doorbell > ring every 8 CQ wraparounds. This driver guarantees one doorbell ring as > soon as the number of consumed CQEs exceeds 4 CQ wraparounds. In pratical nit: practical Flagged by checkpatch.pl --codespell ...
> Subject: Re: [PATCH v2 net] net: mana: Fix doorbell out of order violation and > avoid unnecessary doorbell rings > > On Wed, Aug 07, 2024 at 04:17:06PM -0700, longli@linuxonhyperv.com > wrote: > > From: Long Li <longli@microsoft.com> > > > > After napi_complete_done() is called when NAPI is polling in the > > current process context, another NAPI may be scheduled and start > > running in softirq on another CPU and may ring the doorbell before the > > current CPU does. When combined with unnecessary rings when there is > > no need to arm the CQ, it triggers error paths in the hardware. > > > > This patch fixes this by calling napi_complete_done() after doorbell > > rings. It limits the number of unnecessary rings when there is no need > > to arm. MANA hardware specifies that there must be one doorbell ring > > every 8 CQ wraparounds. This driver guarantees one doorbell ring as > > soon as the number of consumed CQEs exceeds 4 CQ wraparounds. In > > pratical > > nit: practical > > Flagged by checkpatch.pl --codespell > > ... Thank you! Will fix it.
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c index d2f07e179e86..f83211f9e737 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -1788,7 +1788,6 @@ static void mana_poll_rx_cq(struct mana_cq *cq) static int mana_cq_handler(void *context, struct gdma_queue *gdma_queue) { struct mana_cq *cq = context; - u8 arm_bit; int w; WARN_ON_ONCE(cq->gdma_cq != gdma_queue); @@ -1799,16 +1798,23 @@ static int mana_cq_handler(void *context, struct gdma_queue *gdma_queue) mana_poll_tx_cq(cq); w = cq->work_done; - - if (w < cq->budget && - napi_complete_done(&cq->napi, w)) { - arm_bit = SET_ARM_BIT; - } else { - arm_bit = 0; + cq->work_done_since_doorbell += w; + + if (w < cq->budget) { + mana_gd_ring_cq(gdma_queue, SET_ARM_BIT); + cq->work_done_since_doorbell = 0; + napi_complete_done(&cq->napi, w); + } else if (cq->work_done_since_doorbell > + cq->gdma_cq->queue_size / COMP_ENTRY_SIZE * 4) { + /* MANA hardware requires at least one doorbell ring every 8 + * wraparounds of CQ even if there is no need to arm the CQ. + * This driver rings the doorbell as soon as we have exceeded + * 4 wraparounds. + */ + mana_gd_ring_cq(gdma_queue, 0); + cq->work_done_since_doorbell = 0; } - mana_gd_ring_cq(gdma_queue, arm_bit); - return w; } diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h index 6439fd8b437b..7caa334f4888 100644 --- a/include/net/mana/mana.h +++ b/include/net/mana/mana.h @@ -275,6 +275,7 @@ struct mana_cq { /* NAPI data */ struct napi_struct napi; int work_done; + int work_done_since_doorbell; int budget; };