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 9BAF3A00C2; Thu, 3 Feb 2022 11:42:46 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EF35C4014F; Thu, 3 Feb 2022 11:42:45 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id B396B40143 for ; Thu, 3 Feb 2022 11:42:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643884964; x=1675420964; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=W8002eYG1HwkoH8BbD6d6kjic0ejTFl91ZJZCVgPuAE=; b=l2BXu2CVTMNte4hPqFMcd5ASZjRUXum5kfeDWSDqOS1U7NCwamVHQbbn NOaZ+iuVBugG0wjJVWn6U/qmu2YmsWPBmGmYmy6lB0ZeUgTck5krOUjbb GhFu9jYF/IvaHBCaWJpaED4jsArrAE4PRgkyeYIw8K9EiesZMKvj+7p80 jvqdWjBpFXF+5/PWFsMgOvcFrYTTFxiBunI/+Dtp3AoUaR8GX/bMTmkk6 mROmkpcWXCfzxj3Wljv74Hp0A881pvROTbom9BnUy/sP4xLsul6igCMk9 ObKOhwg2wMQtJRJcLBV1A42Ktvvj0EfNrt4ysLEenH81RgIjyHvXeLXH8 A==; X-IronPort-AV: E=McAfee;i="6200,9189,10246"; a="308846668" X-IronPort-AV: E=Sophos;i="5.88,339,1635231600"; d="scan'208";a="308846668" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Feb 2022 02:42:43 -0800 X-IronPort-AV: E=Sophos;i="5.88,339,1635231600"; d="scan'208";a="535165503" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.19.95]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 03 Feb 2022 02:42:42 -0800 Date: Thu, 3 Feb 2022 10:42:38 +0000 From: Bruce Richardson To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Stephen Hemminger , Sean Morrissey , Reshma Pattan , dev@dpdk.org Subject: Re: [PATCH v6 20/50] pdump: remove unneeded header includes Message-ID: References: <20220117201943.873922-1-sean.morrissey@intel.com> <20220202094802.3618978-1-sean.morrissey@intel.com> <20220202094802.3618978-21-sean.morrissey@intel.com> <20220202075458.3f8cd2c1@hermes.local> <98CBD80474FA8B44BF855DF32C47DC35D86E6B@smartserver.smartshare.dk> <20220202092836.5e1aadbb@hermes.local> <98CBD80474FA8B44BF855DF32C47DC35D86E6C@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D86E6C@smartserver.smartshare.dk> 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 On Wed, Feb 02, 2022 at 07:21:33PM +0100, Morten Brørup wrote: > > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > > Sent: Wednesday, 2 February 2022 18.29 > > > > On Wed, 2 Feb 2022 17:03:44 +0000 > > Bruce Richardson wrote: > > > > > On Wed, Feb 02, 2022 at 05:45:47PM +0100, Morten Brørup wrote: > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > > > > Sent: Wednesday, 2 February 2022 17.01 > > > > > > > > > > On Wed, Feb 02, 2022 at 07:54:58AM -0800, Stephen Hemminger > > wrote: > > > > > > On Wed, 2 Feb 2022 09:47:32 +0000 > > > > > > Sean Morrissey wrote: > > > > > > > > > > > > > These header includes have been flagged by the iwyu_tool > > > > > > > and removed. > > > > > > > > > > > > > > Signed-off-by: Sean Morrissey > > > > > > > --- > > > > > > > > [...] > > > > > > > > > > > > > > > > > diff --git a/lib/pdump/rte_pdump.h b/lib/pdump/rte_pdump.h > > > > > > > index 6efa0274f2..41c4b7800b 100644 > > > > > > > --- a/lib/pdump/rte_pdump.h > > > > > > > +++ b/lib/pdump/rte_pdump.h > > > > > > > @@ -13,8 +13,6 @@ > > > > > > > */ > > > > > > > > > > > > > > #include > > > > > > > -#include > > > > > > > -#include > > > > > > > #include > > > > > > > > > > > > > > #ifdef __cplusplus > > > > > > > > > > > > This header does use rte_mempool and rte_ring in > > rte_pdump_enable(). > > > > > > Not sure why IWYU thinks they should be removed. > > > > > > > > > > Because they are only used as pointer types, not as structures > > > > > themselves. > > > > > Normally in cases like this, I would put in just "struct > > rte_mempool;" > > > > > at > > > > > the top of the file rather than including a whole header just for > > one > > > > > structure. > > > > > > > > I don't think we should introduce such a hack! > > > > If a module uses something from a library, it makes sense to > > include the header file for the library. > > > > > > > > Putting in "struct rte_mempool;" is essentially copy-pasting from > > the library, although only a structure. What happens if the type > > changes or disappears, or depends on some #ifdef? It could have one > > type in some cases and another type in other cases - e.g. the atomic > > counters in the mbuf once had different types, depending on compile > > time flags. The copy-pasted code would not get fixed if the type > > evolved over time. > > > > > > By "struct rte_mempool;" I mean literally just that. All it does is > > > indicate that there is a structure defined somewhere else that will > > be used > > > via pointer in the file later on. > > Yes, I did understand that. Like a forward declaration, often used in structures referencing each other. > > > > There is no copy-pasting involved and the > > > reference does not need to change as the structure changes. > > I meant that the rte_mempool itself could change type, so it is no longer a structure. Then the "copy-pasted" forward declaration will allow the code to compile, not catching that the type has changed. > > The refcnt member of the mbuf structure changed over time from being a structure (specifically rte_atomic_t, which was a typedef) to being a simple uint16_t. So types changing over time does happen in the real world, also in DPDK. > If the type does change, then it will still be caught via compilation error, since the corresponding .c file will have to include the full mempool header to access the internals of the data structures. That will trigger a compilation failure on the function in the .c file, requiring the parameter type to change from e.g. struct to union. Overall, though, if a type does change, one would expect that a global search replace would be used throughout the whole codebase, so I consider the possibilities of problems here to be very, very remote. /Bruce