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 63BE8457C4; Wed, 14 Aug 2024 17:04:26 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 54398402C4; Wed, 14 Aug 2024 17:04:26 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id EEE0D40274 for ; Wed, 14 Aug 2024 17:04:24 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id C97292099B; Wed, 14 Aug 2024 17:04:24 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: DPDK configuration options Date: Wed, 14 Aug 2024 17:04:21 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F636@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: DPDK configuration options Thread-Index: AdruPEDVZZ5BthTURZWhMV4kAbHHygAE/ImQ References: <98CBD80474FA8B44BF855DF32C47DC35E9F62D@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" Cc: 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 > From: Bruce Richardson [mailto:bruce.richardson@intel.com] >=20 > On Wed, Aug 14, 2024 at 11:27:58AM +0200, Morten Br=F8rup wrote: > > DPDK has many configuration options. > > > > There are four levels of visibility: > > > > 1. Some are changed by passing command line options to meson. > > 2. Some are changed by modifying their values in = config/rte_config.h. > > 3. Some are changed by adding them to config/rte_config.h, but you = have to > magically know of their existence; e.g. RTE_ENABLE_ASSERT, = RTE_MALLOC_DEBUG > and RTE_LIBRTE_MBUF_DEBUG. > > 4. Some are hidden away in drivers, typically driver specific = options. > > > > And many of the configuration options are not even documented = anywhere in > the code; they are just used by the code. > > > > It seems the level of visibility is currently determined by how = "exotic" the > option is considered to be. I think this is the wrong criteria. > > > > There's also a expectation that a person building DPDK doesn't have = to > modify config/rte_config.h. I think this is a false expectation; if = you are > qualified to build DPDK and tweak it along the way, you certainly = understand > how to modify a header file, and there is no good reason to pass = simple > configuration values (e.g. max_ethports, mbuf_refcnt_atomic and > pkt_mbuf_headroom) 1:1 through meson. > > > > Furthermore, configuration options should not be hidden away or = spread all > over the place. It makes them difficult to find and modify. > > > > Optimally, we would have the same way of configuring DPDK as the = Linux > kernel. > > But I don't see that happening anytime soon. > > So, in the interim, we could use one big configuration file, as = follows: > > > > Options that are not candidates for automatic detection at build = time should > not be level 1, but level 2. (Automatic detection makes sense for e.g. > max_lcores, so that should remain at level 1.) > > > > All level 3 options should be moved to level 2. If there's a = configuration > option, it should be presented (and documented), not hidden away. >=20 > Agreed on this. >=20 > > > > Similarly, level 4 options should be moved to level 2; perhaps = except > options in drivers' "base" directories (code shared by DPDK, Linux = and/or > other systems). >=20 > I'm not sure how visible these need to be. If they are driver = specific, > having them just documented in the specific driver docs is probably = good > enough. Many drivers have "driver specific" (in the driver developer's mind) = configuration parameters. E.g. the VirtIO driver has VIRTIO_MBUF_BURST_SZ (64) [1] as the max = burst returned by rte_eth_rx_burst(), which violates the ethdev API when = more than 64 packets are ready in the queue and rte_eth_rx_burst() is = called with nb_pkts > 64. [1]: = https://elixir.bootlin.com/dpdk/v24.07/source/drivers/net/virtio/virtqueu= e.h#L26 Such parameters are very difficult to notice, unless you go bug hunting = in each specific driver. (Other drivers are likely to have similar = issues; I know some of them have been fixed already. I only picked the = VirtIO driver because it was fresh in my mind, having looked at it = recently.) If those parameters were public and easily visible, we might notice that = some parameters are repeated for multiple drivers, and might even = benefit from being consolidated into a shared parameter. Some parameters are truly driver specific, e.g. using 16 or 32 byte NIC = descriptors. However, if the parameters are there to be tweaked for exotic use cases = (except development of the driver itself), I prefer having them visible = and easy to locate, rather than hidden away. Rte_common.h does have a section for driver defines, so let's use it. >=20 > > > > Each option should have a comment briefly describing what it does. > > > Agreed. >=20 > Taking a step back from the specifics of what options go where, we do = need > to decide overall how we want to manage build options. For example: >=20 > * In the past, we had loads of build options in a flag config file, = but > this turned out to have major issues around validation and didn't = seem > well liked. > * Back when the build system was changed from make to meson/ninja, the > general consensus was that we wanted to - as far as possible - move = away > from build options, because it was impossible to validate all build > combinations, and it was very easy to have broken code inside ifdefs = that > was never even compile-tested. Also, build options didn't work for = distro > targets, where one build was all that was done. Yes, I remember the discussions regarding the two bullet points above. It was decided to try to reduce the number of build time configuration = options in favor of runtime configuration options. I'm not asking to change that decision in any way. > * Since then, though, even though we have had more runtime = configurability > - we have seen a constant increase in build options too. Yes, reality kicked in! Some things just don't make sense being runtime configurable. I'm arguing that: - Hiding build configuration options doesn't make them go away, and - The concept of "how exotic is this configuration parameter?" is not = viable. > * Within build options, not all options are equal. For example, = numeric > values which just affect e.g. array sizes such as number ethdevs, = are > probably pretty harmless from a testing viewpoint, and may need to = be > treated differently from build options, e.g. debug ones, which > enable/disable code blocks and can therefore introduce subtle issues = or > hide problems in disabled code. Current testing only covers one set of configuration parameter values, = the default configuration parameters. It doesn't affect test coverage if those parameters are set at runtime = (by the test case) or at build time. But more importantly: - Making the build configuration parameters more or less visible doesn't = change the test coverage. - It doesn't change the test coverage if the build time configuration = parameters are stored in one or more header files, or if they are given = as parameters to meson. If we want to improve test coverage, we could add more sets of = configuration parameters, e.g. a set with all debug parameters enabled, = and a set with all parameters using randomized values. Having one = configuration file might make that easier. > * Within the mechanisms of build options, the main issue I have with = using > rte_config.h is that it is a file shipped with dpdk and included in = the > repository. That means that any local changes to it get overwritten = with > any new DPDK release or update. Any developer qualified to build DPDK from scratch and modify the DPDK = configuration file knows this. And knows how to deal with it in his = development environment. Having all configuration parameters in one file makes it easy for the = developer to detect new or changed parameters with new releases/updates. > If we want to have such a file-based > approach, I think we need to change things so that we have support = for e.g. > an rte_config_local.h file which, if present, is used to provide = local > overrides for the rte_config settings. The exact mechanism by which = such > a scheme might work I'm not too clear on yet, though! I get your point here, and if a local override config file makes it = easier, I'm not objecting to it. But then we might need to consider a = hierarchy of these, e.g. a "distro" specific override, and then the = developer's own override. It might add more complexity than we foresee. If we are not going all the way to a Linux Kernel-like configuration = system, I'm in favor of KISS and going for one rte_config.h file = (modifiable by the developer). >=20 > Just my 2c. at this point. >=20 > /Bruce