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 A4566A00C2; Thu, 5 Jan 2023 23:06:30 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8537A40697; Thu, 5 Jan 2023 23:06:30 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 9F6514067C for ; Thu, 5 Jan 2023 23:06:28 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id EABF320B928C; Thu, 5 Jan 2023 14:06:27 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com EABF320B928C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1672956387; bh=aADfNuZEd2Lmn4GtOdiUGtGgQA+F1RqUg663ufNMJ58=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DgReA4Jvb+cPQS/0Df/DWDGVfF1tggezSEf8Sj1391ok8YhyOlBAqRKEjgM5nC6J7 j4K3he/J+KCxs0Pum3WWxpsfnZYTdQVgbrPtQkaFZBdlrkIbtUSciGBoExsTvxr3Cp b8aqmdL/jTyKI76Q4yMajDazzXAct3z1tvhA87CA= Date: Thu, 5 Jan 2023 14:06:27 -0800 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Thomas Monjalon , dev@dpdk.org, david.marchand@redhat.com Subject: Re: [PATCH v2 1/2] eal: provide leading and trailing zero bit count abstraction Message-ID: <20230105220627.GB15559@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1669241687-18810-1-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87520@smartserver.smartshare.dk> <20221128172756.GC28869@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <3140231.5fSG56mABF@thomas> <20230105172349.GC9408@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20230105172712.GD9408@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20230105205746.GA15559@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D8762B@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: <98CBD80474FA8B44BF855DF32C47DC35D8762B@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 Thu, Jan 05, 2023 at 10:34:55PM +0100, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Thursday, 5 January 2023 21.58 > > > > On Thu, Jan 05, 2023 at 09:27:12AM -0800, Tyler Retzlaff wrote: > > > On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote: > > > > > > oh! not a problem. i'm very keen to catch any mistakes, thought > > i had > > > > > > missed something. > > > > > > > > > > I think we should move all bit-related functions together. > > > > > Please could you add another patch to your series > > > > > moving "ms1b"/"bsf"/"fls" functions in this file? > > > > > > > > > > > > > > > > > > okay, so there is already a rte_bitops.h. i guess everything should > > go > > > > there including the leading/trailing count functions instead of > > adding a > > > > new header. > > > > > > > > i'll introduce a new patch to the series that gathers the existing > > > > functions into rte_bitops.h and place the new functions there too. > > > > > > > > thanks > > > > > > just as a further follow up, you do understand that this is > > technically > > > an api break? > > > > > > moving functions from rte_common.h to rte_bitops.h will make > > translation > > > units that included only rte_common.h but used these functions will > > > fail to compile without being updated to include rte_bitops.h. > > > > > > anyway, i'll submit v3 with this change anyway. > > > > so when attempting to do this it became immediately obvious that moving > > just the bit op functions out is going to create a circular dependency > > between rte_common.h, rte_bitops.h > > > > once the bit ops are moved out of common there are still other inline > > functions that remain in comman that require bringing bitops back in, > > but bitops depends on common. > > > > my compromise will be to break log2 and pow2 inline functions into > > their > > own files to break the cycle (common no longer depends on bitops). i'll > > submit patches for this but it ends up touching a lot more of the > > tree to add back includes for log/pow inline use. > > > > alternatively i can just not move the remaining bit manipulation > > functions, let me know which is preferred. > > It seems that no perfect solution exists, so we will have to live with a compromise. Here is another proposal for a compromise, for yours and Thomas's consideration: > > I noticed that rte_bitops.h is mainly for setting/getting bits, used for accessing hardware. > > Your functions are mathematical functions, and so are the similar functions in rte_common.h (which is why it makes sense to keep them together with yours). If we cannot clean up rte_common.h by moving them out, perhaps we should accept the current situation (until we find a way to move them out) and just add your mathematical functions where the existing mathematical functions reside, i.e. in rte_common.h. > > This proposal only makes the existing mess slightly larger; it doesn't create a new kind of mess. so i fudged around a bit to see if i could get a happy medium. i ended up with this. remove include of rte_debug.h from rte_bitops.h * had to remove the RTE_ASSERT from existing rte_bitops.h functions * this breaks a good piece of the cycle debug -> log -> common -> bitops -> debug * deal breaker? i don't think it was right that we were getting all of log, common just for using bitops anyway. move pow2 functions from rte_common.h -> rte_pow2ops.h * new header includes rte_bitops.h move log2 functions from rte_common.h -> rte_log2ops.h * new header includes rte_bitops.h, rte_pow2ops.h include rte_bitops.h, rte_pow2ops.h and rte_log2ops.h back into rte_common.h * this is done to reduce the impact of compatibility break by continuing to expose the pow2/log2/bitops via rte_common.h so we end up with 3 standalone headers, where the whole tree builds without having to add a pile of includes for the new headers. we can later deprecate the exposure of the inline functions when including rte_common.h * one caveat is that there was some contamination coming in via the removed rte_debug.h where rte_bitops.h was used. so technically a break of api too. objections? if this is no good i'll just fold my new functions into rte_common.h and leave the mess for the next person, though i am trying not to do that. thanks for the discussion.