> -----Original Message----- > From: Michał Krawczyk <m...@semihalf.com> > Sent: Friday, June 28, 2019 11:03 AM > To: David Harton (dharton) <dhar...@cisco.com> > Cc: dev@dpdk.org; Marcin Wojtas <m...@semihalf.com>; Tzalik, Guy > <gtza...@amazon.com>; Schmeilin, Evgeny <evge...@amazon.com> > Subject: Re: [PATCH] net/ena: Fix admin cq polling for 32-bit apps > > Hi, > > sorry for the late reply. > > śr., 29 maj 2019 o 23:01 David Harton <dhar...@cisco.com> napisał(a): > > > > Recent modifications to admin command queue polling logic did not > > support 32-bit applications. Updated the driver to work for 32 or 64 > > bit applications as well as avoiding roll-over possibility. > > > > Fixes: 3adcba9a89 ("net/ena: update HAL to the newer version") > > > > Signed-off-by: David Harton <dhar...@cisco.com> > > --- > > drivers/net/ena/base/ena_com.c | 10 +++++++--- > > drivers/net/ena/base/ena_plat_dpdk.h | 6 +----- > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/ena/base/ena_com.c > > b/drivers/net/ena/base/ena_com.c index b688067f7..b96adde3c 100644 > > --- a/drivers/net/ena/base/ena_com.c > > +++ b/drivers/net/ena/base/ena_com.c > > @@ -547,10 +547,13 @@ static int > ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c > > struct > > ena_com_admin_queue *admin_queue) { > > unsigned long flags = 0; > > - unsigned long timeout; > > + u32 timeout_ms; > > int ret; > > > > - timeout = ENA_GET_SYSTEM_TIMEOUT(admin_queue- > >completion_timeout); > > + /* Calculate ms granularity timeout from us completion_timeout > > + * making sure we retry once if we have at least 1ms > > + */ > > + timeout_ms = (admin_queue->completion_timeout / 1000) + > > + (ENA_POLL_MS - 1); > > > > while (1) { > > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); @@ > > -560,7 +563,7 @@ static int > ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c > > if (comp_ctx->status != ENA_CMD_SUBMITTED) > > break; > > > > - if (ENA_TIME_EXPIRE(timeout)) { > > + if (timeout_ms < ENA_POLL_MS) { > > ena_trc_err("Wait for completion (polling) > timeout\n"); > > /* ENA didn't have any completion */ > > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); > > @@ -573,6 +576,7 @@ static int > ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c > > } > > > > ENA_MSLEEP(ENA_POLL_MS); > > + timeout_ms -= ENA_POLL_MS; > > This part can be problematic at the very overloaded systems - in that case > the ENA_MSLEEP can take a much longer than ENA_POLL_MS and in this > situation the time spent in this function can't be determined. > That's why we were checking time spent in sleep every ENA_TIME_EXPIRE > macro. > The issue can be observed especially in the kernel drivers, and ena_com is > common file for all ENA drivers.
I don't understand the comment/concern. The previous macros calculate the future cycle count based on a us timeout value (assuming 64 bit apps) and repeat the loop until the command is "submitted" or the current cycle count is greater than the calculated cycle count value sleeping ENA_POLL_MS between each iteration. The new method accomplishes the same thing but instead of using a "cycle count" it uses the number of ms which the poll and sleep actions are based upon. The differences with the new method are: - it uses less instructions - not susceptible to cycle count overrun (admittedly highyl unlikely) - (most importantly) works equally well for 32 or 64 bit apps Can you elaborate on your concern? Thanks, Dave