From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 97329A0487 for ; Mon, 1 Jul 2019 09:24:32 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3EAA5271; Mon, 1 Jul 2019 09:24:31 +0200 (CEST) Received: from mail-ed1-f67.google.com (mail-ed1-f67.google.com [209.85.208.67]) by dpdk.org (Postfix) with ESMTP id 8CF7423D for ; Mon, 1 Jul 2019 09:24:29 +0200 (CEST) Received: by mail-ed1-f67.google.com with SMTP id i11so21683980edq.0 for ; Mon, 01 Jul 2019 00:24:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=j1WzcnbwW/Jye+5LwKYvXXIPd1fxg9HvLtY7YWy2V/Y=; b=R8bN2G3JZm5mkZUq/HpMKX2Ja3tF1rIvrePzd32Sgf15aA9h7TNpkcTRgTYiA4IYEF TrzE0J8jAUJ8Aix7nk9ACVT9v8usZvDUpKdnJA+1Ex6PDKAEFWTRThZq+A7QHAbUOfS0 CgRn+SCXYDXIBqkHyBGZawONyCPbuUjGCRVozXtHzhQ+yMYcANsLVa9SiFpsubsvBvBI xWTgZBIWq/bNMtY/2M4yrAkfuG+nLvKdeNYMk24WD0TMccdEE3PKo1BTx7p5RFICe1RT 5NmGScTF3cOTrY8n3+utidxFZFPvfx6LDsxkILm1PiVTB/t3TueMulr9MZ+s0szoDXKw 9RwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=j1WzcnbwW/Jye+5LwKYvXXIPd1fxg9HvLtY7YWy2V/Y=; b=D9mJgfXkn8TzV3bB1fivRisCIw8CpocKkHsFLc3Rm24mABTcqlrPPyYt39k9pM1tXY mxHkHPJN5+f5Vd3Sx/Hhi8iAS3KAmpoczIeoX2BLKGIcWZoaM6qmLOyCq5c5dGPOp6PL 0L+Om8VqLjwbAwRvlHif3RhL5/g8xOUNAEMsxdqW1Wn2jAAYdAj8xffYUBWD0sFCoCn/ N/8x0nKxub9GJ84Mg+cFu+biPcNBoQpSrMoemJKYtRL587XdBHq9F3Bb5nF4ybbl/GNJ kb/wkVaNJhl4OrSN9fkysndSuUYXlnFLlsiXEU9sOX4hJqdLXkI28qZgO04QukeYuRbq CmgA== X-Gm-Message-State: APjAAAXwHkvPgmF9+EgVj/6WG6yDFoyzsvsgxYP29+hLU8zt27XP4o1L XphSASSP8cnL4Th9aecCGf4vEjDgEvsNjyXWIC2nwQ== X-Google-Smtp-Source: APXvYqzY/1m912bu2bgoLw9OSrKWscxFeKQRTS6lvE17OC7JkuW5PNUDNG6w+IrxVQI9wK8eqerR7tI4RZl6r8learo= X-Received: by 2002:a17:906:4354:: with SMTP id z20mr21220037ejm.163.1561965868820; Mon, 01 Jul 2019 00:24:28 -0700 (PDT) MIME-Version: 1.0 References: <20190529210139.26766-1-dharton@cisco.com> In-Reply-To: From: =?UTF-8?Q?Micha=C5=82_Krawczyk?= Date: Mon, 1 Jul 2019 09:24:17 +0200 Message-ID: To: "David Harton (dharton)" Cc: "dev@dpdk.org" , Marcin Wojtas , "Tzalik, Guy" , "Schmeilin, Evgeny" , "Belgazal, Netanel" , "Kiyanovski, Arthur" , "Chauskin, Igor" , "Matushevsky, Alexander" , sameehj@amazon.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH] net/ena: Fix admin cq polling for 32-bit apps X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" + folks responsible for ENA on other platforms as this code touches every ENA target pt., 28 cze 2019 o 17:46 David Harton (dharton) napisa= =C5=82(a): > > > > > -----Original Message----- > > From: Micha=C5=82 Krawczyk > > Sent: Friday, June 28, 2019 11:03 AM > > To: David Harton (dharton) > > Cc: dev@dpdk.org; Marcin Wojtas ; Tzalik, Guy > > ; Schmeilin, Evgeny > > Subject: Re: [PATCH] net/ena: Fix admin cq polling for 32-bit apps > > > > Hi, > > > > sorry for the late reply. > > > > =C5=9Br., 29 maj 2019 o 23:01 David Harton napisa= =C5=82(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 > > > --- > > > 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 =3D 0; > > > - unsigned long timeout; > > > + u32 timeout_ms; > > > int ret; > > > > > > - timeout =3D ENA_GET_SYSTEM_TIMEOUT(admin_queue- > > >completion_timeout); > > > + /* Calculate ms granularity timeout from us completion_timeou= t > > > + * making sure we retry once if we have at least 1ms > > > + */ > > > + timeout_ms =3D (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 !=3D 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 -=3D ENA_POLL_MS; > > > > This part can be problematic at the very overloaded systems - in that c= ase > > 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 timeou= t value (assuming 64 bit apps) and repeat the loop until the command is "su= bmitted" or the current cycle count is greater than the calculated cycle co= unt 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? The problem with this solution is that you are assuming that ENA_MSLEEP will always sleep for ENA_POLL_MS which is not true. It can sleep much more in busy systems. The behavior of this function before your changes is minimizing that time by getting current cycles in the ENA_TIME_EXPIRE. In the above solution, we can not determine how much time we've sleepped. It could be ENA_POLL_MS or even 10 second. Thanks, Michal > Thanks, > Dave >