From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C9CFF41D44 for ; Thu, 23 Feb 2023 02:15:11 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A8858431AE; Thu, 23 Feb 2023 02:15:11 +0100 (CET) Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by mails.dpdk.org (Postfix) with ESMTP id 5AB7C40DF6 for ; Thu, 23 Feb 2023 02:15:10 +0100 (CET) Received: by mail-pj1-f52.google.com with SMTP id z20-20020a17090a8b9400b002372d7f823eso6963199pjn.4 for ; Wed, 22 Feb 2023 17:15:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=jmCnqhPrIGzA+3s1hkxqdDn0Yhxk1ReOPwyfoTRzYAw=; b=uvQmacr40+Ua4QvHc6ms53M38YgoP5fGDhFQ/bqHstJOAQlfuQW6e5sb82SzOJR6cZ 4RYZttEuCgpcrSLtibkcg22wUQ27D2LjZ0ze0Tgan5jb7EEGV1lGMATHTDAuu6vJJbI2 5D+iiIr4KdSNn4zPmTQ1UN2XlOdz+14luKCazFbJ7+aSJtYH5GkrvcPr/1jguqAlOdOc GXnT0Pqq4O7+uPigT5//L5EmyQtm3cpejse8w6a5J3VDoRflMMKgVgRLjot45ADyRg2f rL0T3KNKqnsbzIbGBk5LOnaXicNze6XWG/HxfbJJGgx1662Pqd0DlJUqBF3OFDu3AGp8 74GQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jmCnqhPrIGzA+3s1hkxqdDn0Yhxk1ReOPwyfoTRzYAw=; b=cYXGRqzVV3O9uBqICGN+JrtJjXV880dJaJ1B4AZn1l2Buqcr0cqXxgJGbMkGEVBWuq ILzDywIw19Q1V0McnJwQqzZWFqGlNTSiQSGhu3N7p5yEcFbduOoBl4gKu2nnBRR8cPJ5 pLSoUXhxzhDmu14oDXrM5lWCKCNA2PYQD+7+uzRJxKl/2tNsVC8qFW+hFvTwlEnik9s2 Y+94zLG5S7kz/mSphAhcrIxbp0NICYSgZ4XCfTCsGLGVQ4n4NYdvJuSHZgPOd5PjxmIm GEwBp/JJYumzyhYnhzZeHTgklgT7eB/h5Ht75u6APg5cUoXZEJxtrCbJo1jWQ6GQAmOR CP6Q== X-Gm-Message-State: AO0yUKXxu7Ui+z7I+mcg7qC/5F5Px5xoS55vxiuCBPK0rpLbkxXbr5Fv Hy9tuU6ArDAa4AERuKVxxrqukA== X-Google-Smtp-Source: AK7set+QpKqamkXaEp1HwZbBMkgFbEmnHzx+XsN0hb/4U2t8jdi1vfXjQp0lN7m/hoi7RSu6RM/3Hg== X-Received: by 2002:a17:902:b597:b0:19c:ac96:1dce with SMTP id a23-20020a170902b59700b0019cac961dcemr2477605pls.48.1677114909392; Wed, 22 Feb 2023 17:15:09 -0800 (PST) Received: from hermes.local (204-195-120-218.wavecable.com. [204.195.120.218]) by smtp.gmail.com with ESMTPSA id s6-20020a170902a50600b001967692d6f5sm7261189plq.227.2023.02.22.17.15.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Feb 2023 17:15:09 -0800 (PST) Date: Wed, 22 Feb 2023 17:15:06 -0800 From: Stephen Hemminger To: Honnappa Nagarahalli Cc: Konstantin Ananyev , Fengchengwen , Ruifeng Wang , Ashok Kaladi , "jerinj@marvell.com" , "thomas@monjalon.net" , "dev@dpdk.org" , "s.v.naga.harish.k@intel.com" , "erik.g.carrillo@intel.com" , "abhinandan.gujjar@intel.com" , "stable@dpdk.org" , nd Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup Message-ID: <20230222171506.715c4bbf@hermes.local> In-Reply-To: References: <20230220060839.1267349-1-ashok.k.kaladi@intel.com> <20230220060839.1267349-2-ashok.k.kaladi@intel.com> <4786db4b-63dc-5329-522d-77eb58d4cff4@huawei.com> <20230221090053.14d653bf@hermes.local> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org On Wed, 22 Feb 2023 22:48:25 +0000 Honnappa Nagarahalli wrote: > > -----Original Message----- > > From: Konstantin Ananyev > > Sent: Wednesday, February 22, 2023 4:41 AM > > To: Fengchengwen ; Stephen Hemminger > > ; Ruifeng Wang > > Cc: Ashok Kaladi ; jerinj@marvell.com; > > thomas@monjalon.net; Honnappa Nagarahalli > > ; dev@dpdk.org; > > s.v.naga.harish.k@intel.com; erik.g.carrillo@intel.com; > > abhinandan.gujjar@intel.com; stable@dpdk.org; nd > > Subject: RE: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup > > > > > > > > > -----Original Message----- > > > From: fengchengwen > > > Sent: Wednesday, February 22, 2023 1:07 AM > > > To: Stephen Hemminger ; Ruifeng Wang > > > > > > Cc: Ashok Kaladi ; jerinj@marvell.com; > > > thomas@monjalon.net; Honnappa Nagarahalli > > > ; dev@dpdk.org; > > > s.v.naga.harish.k@intel.com; erik.g.carrillo@intel.com; > > > abhinandan.gujjar@intel.com; stable@dpdk.org; nd > > > Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops > > > setup > > > > > > On 2023/2/22 1:00, Stephen Hemminger wrote: > > > > On Tue, 21 Feb 2023 07:24:19 +0000 > > > > Ruifeng Wang wrote: > > > > > > > >>> -----Original Message----- > > > >>> From: fengchengwen > > > >>> Sent: Monday, February 20, 2023 2:58 PM > > > >>> To: Ashok Kaladi ; jerinj@marvell.com; > > > >>> thomas@monjalon.net > > > >>> Cc: dev@dpdk.org; s.v.naga.harish.k@intel.com; > > > >>> erik.g.carrillo@intel.com; abhinandan.gujjar@intel.com; > > > >>> stable@dpdk.org; Ruifeng Wang > > > >>> Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path > > > >>> ops setup > > > >>> > > > >>> On 2023/2/20 14:08, Ashok Kaladi wrote: > > > >>>> If ethdev enqueue or dequeue function is called during > > > >>>> eth_dev_fp_ops_setup(), it may get pre-empted after setting the > > > >>>> function pointers, but before setting the pointer to port data. > > > >>>> In this case the newly registered enqueue/dequeue function will > > > >>>> use dummy port data and end up in seg fault. > > > >>>> > > > >>>> This patch moves the updation of each data pointers before > > > >>>> updating corresponding function pointers. > > > >>>> > > > >>>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate > > > >>>> structure") > > > >>>> Cc: stable@dpdk.org > > > > > > > > Why is something calling enqueue/dequeue when device is not fully > > started. > > > > A correctly written application would not call rx/tx burst until > > > > after ethdev start had finished. > > > > > > Please refer the eb0d471a894 (ethdev: add proactive error handling > > > mode), when driver recover itself, the application may still invoke > > enqueue/dequeue API. > > > > Right now DPDK ethdev layer *does not* provide synchronization > > mechanisms between data-path and control-path functions. > > That was a deliberate deisgn choice. If we want to change that rule, then I > > suppose we need a community consensus for it. > +1 > Any such synchronization typically requires using load-acquire on data plane, which brings down the performance. But, init time synchronization would not affect the performance (stating the obvious). > > > I think that if the driver wants to provide some sort of error recovery > > procedure, then it has to provide some synchronization mechanism inside it > > between data-path and control-path functions. > > Actually looking at eb0d471a894 (ethdev: add proactive error handling > > mode), and following patches I wonder how it creeped in? > > It seems we just introduced a loophole for race condition with this > > approach... > > It probably needs to be either deprecated or reworked. > > > > > > > > > > > > > Would something like this work better? > > > > > > > > Note: there is another bug in current code. The check for link state > > > > interrupt and link_ops could return -ENOTSUP and leave device in > > indeterminate state. > > > > The check should be done before calling PMD. > > > > > > > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index > > > > 0266cc82acb6..d6c163ed85e7 100644 > > > > --- a/lib/ethdev/rte_ethdev.c > > > > +++ b/lib/ethdev/rte_ethdev.c > > > > @@ -1582,6 +1582,14 @@ rte_eth_dev_start(uint16_t port_id) > > > > return 0; > > > > } > > > > > > > > + if (dev->data->dev_conf.intr_conf.lsc == 0 && > > > > + dev->dev_ops->link_update == NULL) { > > > > + RTE_ETHDEV_LOG(INFO, > > > > + "Device with port_id=%"PRIu16" link update not > > supported\n", > > > > + port_id); > > > > + return -ENOTSUP; > > > > + } > > > > + > > > > ret = rte_eth_dev_info_get(port_id, &dev_info); > > > > if (ret != 0) > > > > return ret; > > > > @@ -1591,9 +1599,7 @@ rte_eth_dev_start(uint16_t port_id) > > > > eth_dev_mac_restore(dev, &dev_info); > > > > > > > > diag = (*dev->dev_ops->dev_start)(dev); > > > > - if (diag == 0) > > > > - dev->data->dev_started = 1; > > > > - else > > > > + if (diag != 0) > > > > return eth_err(port_id, diag); > > > > > > > > ret = eth_dev_config_restore(dev, &dev_info, port_id); @@ -1611,16 > > > > +1617,18 @@ rte_eth_dev_start(uint16_t port_id) > > > > return ret; > > > > } > > > > > > > > - if (dev->data->dev_conf.intr_conf.lsc == 0) { > > > > - if (*dev->dev_ops->link_update == NULL) > > > > - return -ENOTSUP; > > > > - (*dev->dev_ops->link_update)(dev, 0); > > > > - } > > > > - > > > > /* expose selection of PMD fast-path functions */ > > > > eth_dev_fp_ops_setup(rte_eth_fp_ops + port_id, dev); > > > > > > > > + /* ensure state is set before marking device ready */ > > > > + rte_smp_wmb(); > > > > + > > > > rte_ethdev_trace_start(port_id); > > > > + > > > > + /* Update current link state */ > > > > + if (dev->data->dev_conf.intr_conf.lsc == 0) > > > > + (*dev->dev_ops->link_update)(dev, 0); > > > > + > > > > return 0; > > > > } > > > > > > > > > > > > . > > > > > What about making started a real flag (with weak atomic's) and then any dataplane threads should wait for started flag before going into main loop. It would not solve the error recovery case where the device decides to take itself offline. But that design is racy to start with and needs to be redesigned.