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 DC59CA04B5; Wed, 4 Dec 2019 14:54:50 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DF9221BE83; Wed, 4 Dec 2019 14:54:49 +0100 (CET) Received: from mail-ed1-f67.google.com (mail-ed1-f67.google.com [209.85.208.67]) by dpdk.org (Postfix) with ESMTP id EB7A64C99 for ; Wed, 4 Dec 2019 14:54:48 +0100 (CET) Received: by mail-ed1-f67.google.com with SMTP id s10so6734027edi.5 for ; Wed, 04 Dec 2019 05:54:48 -0800 (PST) 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=cnOyb68sPXQSYySLU0PSyQ7X6E/t8dkv3nyR3/6+xxg=; b=sb5Ozf+Zk94EDhaelG0ZbSCmwo+QiNSE+gIyTQGqAebfjeLuGQDKjunyummnrLqB8p 3sHkk/e2CLG+bDgxViB4BXSbsZigRaJTJ7y6PbbE0l0X6lc0RORXu1W/UVY446dR9Cth 4Ds5s8oNJlltQCZ7ZfvRxP7Wd0ODeJuvQTqj4AdCz+ZmkUh0NM5jbnh+OEsy1BlLgUMD bJoFAx2Mu9AwaOlSiBeMVfpsAcuZ3HMCXscgYVQmKZu5Pm+ckylgggbld+6DW2bVcc0a YsEEm6YsLxrFgugWZ873tXCURmhOcIsK37LjJCiT/IL5TquItGfVwMQd9MdRetZ2Mszq 1i4A== 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=cnOyb68sPXQSYySLU0PSyQ7X6E/t8dkv3nyR3/6+xxg=; b=W6aSj9N8o56nluOszGAsYOwmIWfMzLjmJ1u+iM2i5U9nXXZJCWw60xhOzABxsfzCxh 7qu8eIWroy1qybhq/uG/nFKa5QB1BEq/oe49LRXndQrGzdxqXSVV2rmwS2y/hhYLLa+y KXSuIQ9HNw31XNgWf0QZtkgtP/1slyD2pkiqKc2mrjux3vFrmlftHRTTwBCDpDIjXCWu aHo8elwKSdpMZCJ8rfDOhq30Epo5vLceJ7prOhdtW01geheb1gei6CyGNenpVw1wNOt7 AvQfBC3E1j9PcJuoxUvfCfYr+x01Olfo2EjaHKY+w57QoLkfhQRxnU47P7mk8SGumqG9 nOwg== X-Gm-Message-State: APjAAAXg+15wr2JFZ6BbCsOfl6C7LdchHu8MpT4npzPF5Vy6DnUNEjCu 8TwWypG6qpol6yoGZmsKBIQoIoytXTGb0bSBkCpn+Q== X-Google-Smtp-Source: APXvYqyHdg4zCo1MXOa8GFc10Vf1LRblYKOg/8s+HGFSpPawu6S2Df/a1dLvsbvQulKI72haSfh/P/jbCefD3lDDzd8= X-Received: by 2002:aa7:d84b:: with SMTP id f11mr4153180eds.96.1575467688456; Wed, 04 Dec 2019 05:54:48 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: =?UTF-8?Q?Micha=C5=82_Krawczyk?= Date: Wed, 4 Dec 2019 14:54:37 +0100 Message-ID: To: kumaraparameshwaran rathinavel Cc: dev@dpdk.org, "Chauskin, Igor" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] Admin Queue ENA 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" Hi Param, Adding atomic operations to setting/clearing comp ctxt won't help, as there is no race there. The admin queue is designed this way, that only single completion context can be held, so you should serialize access to the rte_eth_stats_get(). If you won't do that, the 2nd thread will try to hold already occupied context and this will result in disabling admin queue by the ena communication layer - you won't be able to send further admin commands. That's intended behavior and it is caused because you are trying to get the context with the occupied flag being set to true. Adding atomic operations there won't change anything, as there will still be a race between the thread that is waiting for the completion (occupied flag already send to true) and another thread, that is trying to send the same command using the same context (can't set occupied to true, as it's already true) - that should never happen. Without totally reworking ena_com admin queue design, we could add lock in ena_stats_get() - but that'll cause unnecessary locking in all of the applications that are using it from the main lcore context and as your design seems to be unique by doing it from multiple threads, maybe you could add a lock to your calls to the rte_eth_stats_get()? Another solution might be using xstats API, which should let you to get statistics from multiple threads as it's not using admin queue for that - all stats are being counter internally in the PMD. Thanks, Michal pt., 29 lis 2019 o 13:01 kumaraparameshwaran rathinavel napisa=C5=82(a): > > Hi Micha=C5=82, > > Thanks for getting back on this. > > In our design we are using multiple cores requesting for rte_eth_stats_ge= t, it is not from one process and hence not serialized. Since in our design= this is not serialized, and hence in get_comp_ctxt() checking for occupied= flag and comp_ctxt_release() are not done atomically which is causing this= issue. Please let me know if my understanding is correct, so that I will f= ix the application in such a way that it is done from one process and not m= ultiple. > > Thanks, > Param. > > On Thu, Nov 28, 2019 at 6:44 PM Micha=C5=82 Krawczyk wr= ote: >> >> Hi Param, >> >> first of all - you are using very old ena_com. This code comes from >> the DPDK version before v18.08. If you have any doubts, please check >> the newer version of the driver and DPDK as the potential bug could be >> already fixed there. >> >> Anyway, if you will look at the function get_comp_ctxt() which is >> called by __ena_com_submit_admin_cmd() to get the completion context, >> there is a check for the context if it's not occupied - in case it is >> (which will be true until comp_ctxt_release() will clear it), the new >> command using the same context cannot be used. So there shouldn't be >> two consumers using the same completion contexts. >> >> In addition, drivers that are using ena_com are sending admin commands >> one at a time during the init, so there shouldn't be even 2 commands >> at a time. The only exception is ena_com_get_dev_basic_stats(), which >> is called from rte_eth_stats_get() context - but if you consider DPDK >> application, it should use it on the management lcore after init, so >> it'll also be serialized. >> >> Thanks, >> Michal >> >> >> >> pt., 8 lis 2019 o 07:02 kumaraparameshwaran rathinavel >> napisa=C5=82(a): >> > >> > Hi Micha=C5=82, >> > >> > Please look at the below function, >> > >> > static int >> > ena_com_wait_and_process_admin_cq_polling( >> > struct ena_comp_ctx *comp_ctx, >> > struct ena_com_admin_queue *admin_queue) >> > { >> > unsigned long flags =3D 0; >> > u64 start_time; >> > int ret; >> > >> > start_time =3D ENA_GET_SYSTEM_USECS(); >> > >> > while (comp_ctx->status =3D=3D ENA_CMD_SUBMITTED) { >> > if ((ENA_GET_SYSTEM_USECS() - start_time) > >> > ADMIN_CMD_TIMEOUT_US) { >> > ena_trc_err("Wait for completion (polling) timeout\n"); >> > /* ENA didn't have any completion */ >> > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); >> > admin_queue->stats.no_completion++; >> > admin_queue->running_state =3D false; >> > ENA_SPINLOCK_UNLOCK(admin_queue->q_lock, flags); >> > >> > ret =3D ENA_COM_TIMER_EXPIRED; >> > goto err; >> > } >> > >> > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); >> > ena_com_handle_admin_completion(admin_queue); >> > ENA_SPINLOCK_UNLOCK(admin_queue->q_lock, flags); >> > } >> > >> > if (unlikely(comp_ctx->status =3D=3D ENA_CMD_ABORTED)) { >> > ena_trc_err("Command was aborted\n"); >> > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); >> > admin_queue->stats.aborted_cmd++; >> > ENA_SPINLOCK_UNLOCK(admin_queue->q_lock, flags); >> > ret =3D ENA_COM_NO_DEVICE; >> > goto err; >> > } >> > >> > ENA_ASSERT(comp_ctx->status =3D=3D ENA_CMD_COMPLETED, >> > "Invalid comp status %d\n", comp_ctx->status); >> > >> > ret =3D ena_com_comp_status_to_errno(comp_ctx->comp_status); >> > err: >> > comp_ctxt_release(admin_queue, comp_ctx); >> > return ret; >> > } >> > >> > This is a case where there are two threads executing admin commands. >> > >> > The occupied flag is set to false in the function comp_ctxt_release. = Let us say there are two consumers of completion context and C1 has a compl= etion context and the same completion context can be used by another consum= er C2 even before the C1 is resetting the occupied flag. >> > >> > This is because the ena_com_handle_admin_completion is done under spin= lock and comp_ctxt_release is not under this spin lock. >> > >> > Thanks, >> > Param >> > >> > On Thu, Oct 24, 2019 at 2:09 PM Micha=C5=82 Krawczyk = wrote: >> >> >> >> sob., 19 pa=C5=BA 2019 o 20:26 kumaraparameshwaran rathinavel >> >> napisa=C5=82(a): >> >> > >> >> > Hi All, >> >> > >> >> > In the ENA poll mode driver I see that every request in the admin q= ueue is >> >> > associated with a completion context and this is preallocated durin= g the >> >> > device initialisation. When the completion context is used we check= for >> >> > occupied to be true in the 16.X version if the occupied flag is set= to true >> >> > we assert and in the latest version I see that this is an error log= . But >> >> > there is a time window where if the completion context would be ava= ilable >> >> > to the other consumer but still the old consumer did not set the oc= cupied >> >> > to false. The new consumer holds the admin queue lock to get the co= mpletion >> >> > context but the update by the old consumer to set the the occupied = flag is >> >> > not done under lock. So should we make sure that the new consumer s= hould >> >> > get the completion context only when the occupied flag is set to fa= lse. Any >> >> > thoughts on this? >> >> >> >> Hi Param, >> >> >> >> Both the producer and the consumer are holding the spinlock while >> >> getting the completion context. If you see any situation where it >> >> isn't (besides the release function), please let me know. >> >> As it is protected by the lock, returning error while completion >> >> context is occupied (and it shouldn't) it fine, as it will stop the >> >> admin queue and allow the DPDK user application to execute the reset >> >> of the device. >> >> >> >> Thanks, >> >> Michal >> >> >> >> > If required I can try to make a patch where the completion context = would be >> >> > available only after setting the occupied flag to false. >> >> > >> >> > Thanks, >> >> > Param.