From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 2/2] net/mlx5: panic when destroying a queue in use Date: Wed, 12 Apr 2017 19:04:28 +0200 Message-ID: <20170412170428.GC3790@6wind.com> References: <6cc1a018-3d2b-e51b-97f3-737b93dccc26@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Nelio Laranjeiro , dev@dpdk.org To: Ferruh Yigit Return-path: Received: from mail-wr0-f179.google.com (mail-wr0-f179.google.com [209.85.128.179]) by dpdk.org (Postfix) with ESMTP id 80CF82C27 for ; Wed, 12 Apr 2017 19:04:37 +0200 (CEST) Received: by mail-wr0-f179.google.com with SMTP id o21so21509065wrb.2 for ; Wed, 12 Apr 2017 10:04:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: <6cc1a018-3d2b-e51b-97f3-737b93dccc26@intel.com> 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 Wed, Apr 12, 2017 at 05:48:40PM +0100, Ferruh Yigit wrote: > On 4/11/2017 4:21 PM, Nelio Laranjeiro wrote: > > Since the queue release API does not allow failures (return value is void) > > and the flow API does not allow a queue to be released as long as a flow > > rule depends on it, the only rational decision to avoid undefined behavior > > is to panic in this situation. > > > > Signed-off-by: Nelio Laranjeiro > > Acked-by: Adrien Mazarguil > > <...> > > > @@ -1248,6 +1249,9 @@ mlx5_rx_queue_release(void *dpdk_rxq) > > rxq_ctrl = container_of(rxq, struct rxq_ctrl, rxq); > > priv = rxq_ctrl->priv; > > priv_lock(priv); > > + if (priv_flow_rxq_in_use(priv, rxq)) > > + rte_panic("Rx queue %p is still used by a flow and cannot be" > > + " removed\n", (void *)rxq_ctrl); > > Actually, this adds exit code to the PMD, not sure this is good idea. > > There was a patch to remove them from libraries. The exit decision > should be belong to the application, not to the PMD, what do you think? I think rte_panic() makes sense in such cases. It's a voluntary crash similar to assert() (BUG_ON() for kernel folks) to avoids memory corruption and other nasty things which unfortunately can occur when applications happen to do things in the wrong order. In this specific case, it is caused by shortcomings in the current API, namely the lack of a return value for this function, which cannot be changed at this stage. This is not a fix as there is not really a commit to blame. > > > for (i = 0; (i != priv->rxqs_n); ++i) > > if ((*priv->rxqs)[i] == rxq) { > > DEBUG("%p: removing RX queue %p from list", > > > -- Adrien Mazarguil 6WIND