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 EE2A0A0C46; Fri, 17 Sep 2021 13:53:18 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 76084406B4; Fri, 17 Sep 2021 13:53:18 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id EA03E40689 for ; Fri, 17 Sep 2021 13:53:16 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 7D0257F4FD; Fri, 17 Sep 2021 14:53:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 7D0257F4FD DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1631879596; bh=P2Jg05+GbYZWQINNNGI8FvECvzYzwJOhNrwYfjK9lZ8=; h=Subject:From:To:Cc:References:Date:In-Reply-To; b=FbIjW8ClGjcInvc0XwjHPPw6ExBJsM9c1bQvCTvw1ZpDPTkXOZOTKfcA2B3ZFQAj7 xtf4ljCHjtWL2u1ZAWS1hUl+LsWzATu6NkpzC0anKoF5PMuydZvcifeO18Fw4GY70+ 9Q1txuwU51ehV0X90dni9XKQfBwF1bwEjmK/3iIU= From: Andrew Rybchenko To: Xueming Li , dev@dpdk.org Cc: Ferruh Yigit , "\"Singh, Aman Deep" , Thomas Monjalon References: <20210727034134.20556-1-xuemingl@nvidia.com> <20210917093915.350863-1-xuemingl@nvidia.com> <20210917093915.350863-2-xuemingl@nvidia.com> <781a12fa-8d14-465f-1cbe-0406a0ab0ae6@oktetlabs.ru> Organization: OKTET Labs Message-ID: <6a1c36f7-b16f-3ee7-1f06-c745bdb0cf87@oktetlabs.ru> Date: Fri, 17 Sep 2021 14:53:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <781a12fa-8d14-465f-1cbe-0406a0ab0ae6@oktetlabs.ru> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 1/2] ethdev: queue release callback optional 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 9/17/21 2:29 PM, Andrew Rybchenko wrote: > On 9/17/21 12:39 PM, Xueming Li wrote: >> Some drivers don't need Rx and Tx queue release callback, make it >> optional. >> >> Signed-off-by: Xueming Li > > LGTM, but please, consider one nit below > > Reviewed-by: Andrew Rybchenko > >> --- >> lib/ethdev/rte_ethdev.c | 48 +++++++++++++++++------------------------ >> 1 file changed, 20 insertions(+), 28 deletions(-) >> >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index daf5ca9242..2f316d1646 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -905,12 +905,11 @@ eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues) >> return -(ENOMEM); >> } >> } else if (dev->data->rx_queues != NULL && nb_queues != 0) { /* re-configure */ >> - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP); >> - >> rxq = dev->data->rx_queues; >> >> - for (i = nb_queues; i < old_nb_queues; i++) >> - (*dev->dev_ops->rx_queue_release)(rxq[i]); >> + if (dev->dev_ops->rx_queue_release != NULL) >> + for (i = nb_queues; i < old_nb_queues; i++) >> + (*dev->dev_ops->rx_queue_release)(rxq[i]); > > Since 'if' body has more than one line, I'd add curly brackets > around to make it a bit easier to read and more robust against > future changes. > > Similar note is applicable to many similar cases in the patch. > Reviewed the next patch I realize one thing: Who is responsible for setting dev->data->rxq[rx_queue_id] to NULL if release callback is not specified. IMHO, it is inconsistent to keep it filled in after release. I think that the generic code in ethdev must care about it regardless callback presence. It means that we need helper function which cares about it in single place. Also it means that we can't optimize loops like this. We need the loop anyway to set all queue pointers to NULL.