From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f52.google.com (mail-lf0-f52.google.com [209.85.215.52]) by dpdk.org (Postfix) with ESMTP id 73EB2C762 for ; Wed, 15 Jun 2016 16:07:22 +0200 (CEST) Received: by mail-lf0-f52.google.com with SMTP id j7so13818045lfg.1 for ; Wed, 15 Jun 2016 07:07:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=Z95rcrLlE/yV2YP9326/wFwPo/ONGPQ/ckmvyqBM9Bw=; b=VeDAbMw7NfQgBWrmJYjNWYiKI1Ht61aLMrKlpW0KuZDCpSF/Znie0Zho9OXKpXOhsp 26VCTgM/hMiz1ONnFnDWBPb3qI/x27zycMSMH7ra7K6R6cl1n4lWWYXaDbKeIsNlhF+c qdxFanDK74ctx2YMt9qh/CNRR85qoJIcJli7/fBJUGM26pOJiiVKtHt48gCY/UOXdDI7 Z2aRhK5fJXY1P8keV+p/sPUIivujuevtRuGF1pp1Spugi8IoyFBKKrnisekELW/ch0G0 39Oppd1f/lzPzJ92lvrTEYwc6LqsOPuoNmuigkD9Qhwlu35IfKocXSVprzWrKp7V5Ycl wJhA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=Z95rcrLlE/yV2YP9326/wFwPo/ONGPQ/ckmvyqBM9Bw=; b=HxQ89b2RAh3UoJ9vXaBPhTcWbMmUCuAlIViNNakyIbqLa9L25GfSds3JE3XfpuZYcM Ta1xqD8TKKrjzUkWvEKEezHIs0/7kfOg1KeOdis1j27jmI1nW205wwm/dpG2oS0BxvGF rh4QWbxfBXjfjSu0lhVC3PuVNvgO6UJzbMrfhepLe+vtX+7AlWG2WIHw3VWuj5AxrGNv Yvx2l8rYaaz/PsEu2+Mkk4kxIMnppoSyx3qg2C3Da6txJsV48+13FgNsv3w0ELfhwRWy PkNnVGmcsOfUZmyxixcDdkRhUGBRxySg7LJrYx5cXYQRWaDUd99+hwanx4+k5w8hTAY6 9yJw== X-Gm-Message-State: ALyK8tIbVPZPBnjBGe7pJtdnpWsqwJeukZcQ1GqfsPGjkGtELKYgP9t2Xmu21a1gt+8DXUeA X-Received: by 10.194.203.226 with SMTP id kt2mr11687027wjc.75.1465999642029; Wed, 15 Jun 2016 07:07:22 -0700 (PDT) Received: from [10.16.0.189] (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id x83sm4440476wmx.9.2016.06.15.07.07.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 15 Jun 2016 07:07:20 -0700 (PDT) To: Bruce Richardson , "Ananyev, Konstantin" References: <1465575534-23605-1-git-send-email-reshma.pattan@intel.com> <10886152.VH5xYhdqG2@xps13> <2601191342CEEE43887BDE71AB97725836B714ED@irsmsx105.ger.corp.intel.com> <2907169.iIEIeOfXh7@xps13> <576146CD.8060008@6wind.com> <2601191342CEEE43887BDE71AB97725836B717B0@irsmsx105.ger.corp.intel.com> <20160615132928.GA11536@bricha3-MOBL3> Cc: Thomas Monjalon , "Pattan, Reshma" , "dev@dpdk.org" From: Ivan Boule Message-ID: <57616118.7080806@6wind.com> Date: Wed, 15 Jun 2016 16:07:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160615132928.GA11536@bricha3-MOBL3> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v9 1/8] ethdev: use locks to protect Rx/Tx callback lists X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Jun 2016 14:07:22 -0000 On 06/15/2016 03:29 PM, Bruce Richardson wrote: > On Wed, Jun 15, 2016 at 12:40:16PM +0000, Ananyev, Konstantin wrote: >> Hi Ivan, >> >>> -----Original Message----- >>> From: Ivan Boule [mailto:ivan.boule@6wind.com] >>> Sent: Wednesday, June 15, 2016 1:15 PM >>> To: Thomas Monjalon; Ananyev, Konstantin >>> Cc: Pattan, Reshma; dev@dpdk.org >>> Subject: Re: [dpdk-dev] [PATCH v9 1/8] ethdev: use locks to protect Rx/Tx callback lists >>> >>> On 06/15/2016 10:48 AM, Thomas Monjalon wrote: >>> >>>>> >>>>>> I think the read access would need locking but we do not want it >>>>>> in fast path. >>>>> >>>>> I don't think it would be needed. >>>>> As I said - read/write interaction didn't change from what we have right now. >>>>> But if you have some particular scenario in mind that you believe would cause >>>>> a race condition - please speak up. >>>> >>>> If we add/remove a callback during a burst? Is it possible that the next >>>> pointer would have a wrong value leading to a crash? >>>> Maybe we need a comment to state that we should not alter burst >>>> callbacks while running burst functions. >>>> >>> >>> Hi Reshma, >>> >>> You claim that the "rte_eth_rx_cb_lock" does not need to be hold in the >>> function "rte_eth_rx_burst()" while parsing the "post_rx_burst_cbs" list >>> of RX callbacks associated with the polled RX queue to safely remove RX >>> callback(s) in parallel. >>> The problem is not [only] with the setting and the loading of "cb->next" >>> that you assume to be atomic operations, which is certainly true on most >>> CPUs. >>> I see the 2 important following issues: >>> >>> 1) the "rte_eth_rxtx_callback" data structure associated with a removed >>> RX callback could still be accessed in the callback parsing loop of the >>> function "rte_eth_rx_burst()" after having been freed in parallel. >>> >>> BUT SUCH A BAD SITUATION WILL NOT CURRENTLY HAPPEN, THANKS TO THE NICE >>> MEMORY LEAK BUG in the function "rte_eth_remove_rx_callback()" that >>> does not free the "rte_eth_rxtx_callback" data structure associated with >>> the removed callback ! >> >> Yes, though it is documented behaviour, someone can probably >> refer it as a feature, not a bug ;) >> > > +1 > This is definitely not a bug, this is absolutely by design. One may argue with > the design, but it was done for a definite reason, so as to avoid paying the > penalty of having locks. It pushes more responsibility onto the app, but it > does allow the app to choose the best solution for managing the freeing of > memory for its situation. The alternative is to force all apps to pay the cost > of having locks, even if better options for freeing the memory are available. > > /Bruce > -1 (not to say 0xFFFFFFFF) This is definitely an API design bug ! I would say that if you don't know how to free a resource that you allocate, it is very likely that you are wrong allocating it. And this is exactly what happens here with RX/TX callback data structures. This problem can easily be addressed by just changing the API as follows: Change void * rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id, rte_rx_callback_fn fn, void *user_param) to int rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id, struct rte_eth_rxtx_callback *cb) In addition of solving the problem, this approach makes the API consistent and let the application allocate "rte_eth_rxtx_callback" data structures through any appropriate mean. Regards, Ivan -- Ivan Boule 6WIND Development Engineer