From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1FC17C0650E for ; Mon, 1 Jul 2019 12:53:01 +0000 (UTC) Received: from dpdk.org (dpdk.org [92.243.14.124]) by mail.kernel.org (Postfix) with ESMTP id 8193720B7C for ; Mon, 1 Jul 2019 12:53:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=semihalf-com.20150623.gappssmtp.com header.i=@semihalf-com.20150623.gappssmtp.com header.b="eWVWjhA0" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8193720B7C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=semihalf.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=dev-bounces@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 07C8F5587; Mon, 1 Jul 2019 14:52:59 +0200 (CEST) Received: from mail-ed1-f68.google.com (mail-ed1-f68.google.com [209.85.208.68]) by dpdk.org (Postfix) with ESMTP id 040A1374C for ; Mon, 1 Jul 2019 14:52:57 +0200 (CEST) Received: by mail-ed1-f68.google.com with SMTP id k8so23304969edr.11 for ; Mon, 01 Jul 2019 05:52:57 -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=oRFxul0cKYg0q1l+JSSrl8V/umjZ8SmfNGYM0beiX4M=; b=eWVWjhA0RaB5ZWFl5q/W+rFAr2xGfj947RiGyyq4c+6aFHNsd591VtSCRRm/66JFt/ vV34U1W9IQS3qFIskelNInWG6u/j0lm5Bj4NVfxAVhDvo5YOKKxvAFGDlr6WF2ADBFN2 Ito5zV66X1d5Oc/6edcdBawiEYlBCFjlMy10RL8WtTujVlPB+OeIYWrK6WitXbU282qu 8wX5Im6XmPVZxwNaquFe/oX63Pk3Co6IwK1SHPAq2bBcVzP+kYz9kcq8Vs6Ez92ylGgr uKwRZYA59HmVmsCpbWqEJvLSw1GRvl6NBMVEifAYxblDpb18sfq96WXkIWRGbg3Ii0qQ 6/+A== 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=oRFxul0cKYg0q1l+JSSrl8V/umjZ8SmfNGYM0beiX4M=; b=IHghn8223r4BwdfOUNZV2p0jlnIGNTT3bXdXoCaw3pvgi6umBOhoNCKKUbKgGsLrzA pB7O+Sfb9Jw8MuCfXX1rTUG+QyA6nxQ49j4IUDRcaTlrlHtlk+psIvDUTyu+JQ7GhgdZ 9UtBbFsP5sCVysiscojm5hiMKYFuZqp0AGf00czkR69xB+VJzEj/fazmRGZUxm1gzJEe T5b0mm3sePrC7emGtnQT0Yr5w04mCmngBf/TMp78qhlD9vpLEVV6guUUT47KTU/EqyG6 tdPonIuPpOZoJ51/VS60LCTVN6XUhPJcv0m9+B9y7IzoyjRZZH+11vnvo0fNfVLt3uBh JkHA== X-Gm-Message-State: APjAAAV97GNA0rXjVFVV7KQrnkAoCzvvPJT3FBY299z9haFHcB4lEm2K O6zsOLbgRpQULYIl32JfSkKiu2kKMiMl3/zzl2Sy6A== X-Google-Smtp-Source: APXvYqzcg0dt8x2Pb9p1ZqhiKYuOqy5Y/97S372bk/TPdw6v44Dp9srCxIp/hGmsqQ5iWrDJgHBQ9IBqE1Mvi+Gh2nA= X-Received: by 2002:a17:907:20ed:: with SMTP id rh13mr23098269ejb.34.1561985577564; Mon, 01 Jul 2019 05:52:57 -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 14:52:46 +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" pon., 1 lip 2019 o 14:01 David Harton (dharton) napisa= =C5=82(a): > > > > > -----Original Message----- > > From: Micha=C5=82 Krawczyk > > Sent: Monday, July 01, 2019 3:24 AM > > 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 > > Subject: Re: [PATCH] net/ena: Fix admin cq polling for 32-bit apps > > > > + 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 napi= sa=C5=82(a): > > > > > > > > > > Recent modifications to admin command queue polling logic did not > > > > > support 32-bit applications. Updated the driver to work for 32 o= r > > > > > 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 10064= 4 > > > > > --- 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_timeout > > > > > + * 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 case the ENA_MSLEEP can take a much longer than ENA_POLL_MS an= d > > > > in this situation the time spent in this function can't be determin= ed. > > > > 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 comm= and > > is "submitted" or the current cycle count is greater than the calculate= d > > cycle count value sleeping ENA_POLL_MS between each iteration. > > > > > > > > > The new method accomplishes the same thing but instead of using a "cy= cle > > count" it uses the number of ms which the poll and sleep actions are ba= sed > > 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 ti= me > > 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, I understand your concern now. It's true that what I added is mu= ch more coarse than what was there because any call to rte_delay_ms() is on= ly guaranteed to wait at lease those ms and can wait longer. > > Kinda scary to think though I ask to wait say 10ms and the context be swi= tched out for 10s. Not really a high-performing or low latency system in t= hat case and if so I'm not sure I see the harm waiting for the coarse amoun= t of time either. > > However, if new approach is truly not desired then the original approach = can preserved if it uses uint64_t to track clock cycles instead of the arch= itecture dependent type currently used. Using uint64_t instead of unsigned long is fine for me. That shouldn't affect other platforms as much. But let's hear from others if they are fine with that kind of change. Thanks, Michal > > Regards, > Dave > > > > > Thanks, > > Michal > > > > > Thanks, > > > Dave > > >