RE: DPDK configuration options

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> 
> On Wed, Aug 14, 2024 at 11:27:58AM +0200, Morten Brørup 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.
> 
> Agreed on this.
> 
> >
> > 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).
> 
> 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/virtqueue.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.

> 
> >
> > Each option should have a comment briefly describing what it does.
> >
> Agreed.
> 
> 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:
> 
> * 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).

> 
> Just my 2c. at this point.
> 
> /Bruce