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 2A9A6A0C4E; Fri, 15 Oct 2021 15:40:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A93C5411CB; Fri, 15 Oct 2021 15:40:29 +0200 (CEST) Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by mails.dpdk.org (Postfix) with ESMTP id 4EB33410F1 for ; Fri, 15 Oct 2021 15:40:28 +0200 (CEST) Received: by mail-wr1-f44.google.com with SMTP id u18so26706772wrg.5 for ; Fri, 15 Oct 2021 06:40:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=1XnMwFaTaT4KDfbX5OinGIihzEnjY88at8BjhEnGqIg=; b=QWtQnBtv5leeZzoZFQqnkL1rMmZwIZeejaRyWp3Ruy2QUKUdxRuG+jHBASslh4DEaE oJpyKdFy2UIwg/4Dd6zAQT/Va4/jna0h9LRe17V7IBnVDcylOsGMmjtwJxh3MhPVfJkw hnK47t1qrcQffchOaCSY8l9P0tGJLjp+sIZGQW/s1Hi5h4l++NI28vjzAR7BYO079Dw1 +CGOL6BXwzxzwlyAA+Mpfq8bOFAAbSld9cGJ/D/yZRI7/bOpHflU8JC7L4G3RtH5K7ie jNK3SVfeVnWOJZdBmblLiflb/nfYfyF/pbfVkVagSMe/wVc0/HiCDzCYGZDw9ke6Vyq5 /i6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=1XnMwFaTaT4KDfbX5OinGIihzEnjY88at8BjhEnGqIg=; b=eSiwrPjh7SNR3Pspv5fs+6oM4i9gbVcSQagNUh5wDXSgSwX/3QJylSQnUpNOfF0iAu HKXg1eZOHvOQRZsWiiMyeJUd0Ul+SeYkQb+VaFOHudYJ+lETYPWntNf6yEAs7auQL7IK 5tz5xbCNjY68rPNnAQxPeJQuY17SyIFzRe0nuwVM0Sb40plAxb1bIbj0E42LKg3xCskF tizl6/rgQMvwSBD5RhU4PeQ1QG+MP00rrjwBGjhljBaKjBydoSF8JD42XcU33qq64OwQ 9kFBZdh1q0s+Jzjx8UxCE6gDnktzhK8dlAZj/tb6ey/+7LlbxgZw/+5t9lgzQJ8/Spzk NCqg== X-Gm-Message-State: AOAM5310aOCdj0SqwVJ813RMjjkPjgluKUuJ6PNYOS5aJQ0vEWC9l07/ RymWR4haRstkO8parZW0bv18lQ== X-Google-Smtp-Source: ABdhPJz+D9UeSuQKjXwPzS19CcZHk6Q9ubMFZImPtk3rgoHfAtpNNwO3PJCmV2wd7Yf6t4GLx/frCw== X-Received: by 2002:adf:a51e:: with SMTP id i30mr14825735wrb.206.1634305228034; Fri, 15 Oct 2021 06:40:28 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id a5sm4995655wrn.71.2021.10.15.06.40.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Oct 2021 06:40:26 -0700 (PDT) Date: Fri, 15 Oct 2021 15:40:26 +0200 From: Olivier Matz To: Dmitry Kozlyuk Cc: "dev@dpdk.org" , Andrew Rybchenko , Matan Azrad , Ray Kinsella , Anatoly Burakov Message-ID: References: <20211012000409.2751908-1-dkozlyuk@nvidia.com> <20211013110131.2909604-1-dkozlyuk@nvidia.com> <20211013110131.2909604-2-dkozlyuk@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v4 1/4] mempool: add event callbacks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" On Fri, Oct 15, 2021 at 01:07:42PM +0000, Dmitry Kozlyuk wrote: [...] > > > +static void > > > +mempool_event_callback_invoke(enum rte_mempool_event event, > > > + struct rte_mempool *mp) > > > +{ > > > + struct mempool_callback_list *list; > > > + struct rte_tailq_entry *te; > > > + void *tmp_te; > > > + > > > + rte_mcfg_tailq_read_lock(); > > > + list = RTE_TAILQ_CAST(callback_tailq.head, mempool_callback_list); > > > + RTE_TAILQ_FOREACH_SAFE(te, list, next, tmp_te) { > > > + struct mempool_callback *cb = te->data; > > > + rte_mcfg_tailq_read_unlock(); > > > + cb->func(event, mp, cb->user_data); > > > + rte_mcfg_tailq_read_lock(); > > > > I think it is dangerous to unlock the list before invoking the callback. > > During that time, another thread can remove the next mempool callback, and > > the next iteration will access to a freed element, causing an undefined > > behavior. > > > > Is it a problem to keep the lock held during the callback invocation? > > > > I see that you have a test for this, and that you wrote a comment in the > > documentation: > > > > * rte_mempool_event_callback_register() may be called from within the > > callback, > > * but the callbacks registered this way will not be invoked for the same > > event. > > * rte_mempool_event_callback_unregister() may only be safely called > > * to remove the running callback. > > > > But is there a use-case for this? > > If no, I'll tend to say that we can document that it is not allowed to > > create, free or list mempools or register cb from the callback. > > There is no use-case, but I'd argue for releasing the lock. > This lock is taken by rte_xxx_create() functions in many libraries, > so the restriction is wider and, worse, it is not strictly limited. Yes... I honnestly don't understand why every library uses the same lock rte_mcfg_tailq if the only code that accesses the list is in the library itself. Maybe I'm missing something. I have the impression that having only one mempool lock for all usages in mempool would be simpler and more specific. It would allow to keep the lock held while invoking the callbacks without blocking accesses to the other libs, and would also solve the problem described below. > > [...] > > > +int > > > +rte_mempool_event_callback_unregister(rte_mempool_event_callback *func, > > > + void *user_data) > > > +{ > > > + struct mempool_callback_list *list; > > > + struct rte_tailq_entry *te = NULL; > > > + struct mempool_callback *cb; > > > + int ret; > > > + > > > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) { > > > + rte_errno = EPERM; > > > + return -1; > > > + } > > > > The help of the register function says > > * Callbacks will be invoked in the process that creates the mempool. > > BTW, this is another bug, it should be "populates", not "creates". > > > So registration is allowed from primary or secondary process. Can't a > > secondary process destroys the callback it has loaded? > > > > > + > > > + rte_mcfg_mempool_read_lock(); > > > + rte_mcfg_tailq_write_lock(); > > > > I don't understand why there are 2 locks here. > > > > After looking at the code, I think the locking model is already > > incorrect in current mempool code: > > > > rte_mcfg_tailq_write_lock() is used in create and free to protect the > > access to the mempool tailq > > > > rte_mcfg_mempool_write_lock() is used in create(), to protect from > > concurrent creation (with same name for instance), but I doubt it > > is absolutly needed, because memzone_reserve is already protected. > > > > rte_mcfg_mempool_read_lock() is used in dump functions, but to me > > it should use rte_mcfg_tailq_read_lock() instead. > > Currently, doing a dump and a free concurrently can cause a crash > > because they are not using the same lock. > > > > In your case, I suggest to use only one lock to protect the callback > > list. I think it can be rte_mcfg_tailq_*_lock(). > > Thanks, I will double-check the locking.