Re: [dpdk-dev] [PATCH v3 2/2] doc: include config options in testpmd user guide

[Dharmik Thakkar <Dharmik.Thakkar@arm.com>]

> On Jun 22, 2020, at 10:26 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> 19/06/2020 18:13, Dharmik Thakkar:
>>> On Jun 19, 2020, at 10:43 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
>>> 19/06/2020 17:38, Dharmik Thakkar:
>>>>> On Jun 17, 2020, at 2:48 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>> 17/06/2020 20:21, Ferruh Yigit:
>>>>>> On 5/20/2020 4:20 AM, Dharmik Thakkar wrote:
>>>>>>> Update testpmd documentation to include RECORD configuration options,
>>>>>>> CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES and
>>>>>>> CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS.
>>>>>>> 
>>>>>>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>>>>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>> ---
>>>>>>> v3:
>>>>>>> - Replace config/common_base with build/.config (Thomas)
>>>>>>> 
>>>>>>> v2:
>>>>>>> - Remove extra '#'.
>>>>>>> ---
>>>>>>> doc/guides/testpmd_app_ug/build_app.rst | 12 ++++++++++++
>>>>>>> 1 file changed, 12 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/doc/guides/testpmd_app_ug/build_app.rst b/doc/guides/testpmd_app_ug/build_app.rst
>>>>>>> index d1ca9f3d19a9..8c9aaa83187f 100644
>>>>>>> --- a/doc/guides/testpmd_app_ug/build_app.rst
>>>>>>> +++ b/doc/guides/testpmd_app_ug/build_app.rst
>>>>>>> @@ -21,6 +21,18 @@ The basic compilation steps are:
>>>>>>> 
>>>>>>>       export RTE_TARGET=x86_64-native-linux-gcc
>>>>>>> 
>>>>>>> +#.  If required, enable configuration options. For example:
>>>>>>> +
>>>>>>> +    .. code-block:: console
>>>>>>> +
>>>>>>> +        cd to the top-level DPDK directory
>>>>>>> +        sed -i 's,\(CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES\)=n,\1=y,' build/.config
>>>>>>> +        sed -i 's,\(CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS\)=n,\1=y,' build/.config
>>>>>>> +
>>>>>>> +    Enabling CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES enables measurement of CPU cycles.
>>>>>>> +
>>>>>>> +    Enabling CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS enables display of RX and TX bursts.
>>>>>>> +
>>>>>>> #.  Build the application:
>>>>>>> 
>>>>>>>   .. code-block:: console
>>>>>>> 
>>>>>> 
>>>>>> Hi Dharmik,
>>>>>> 
>>>>>> This patch waiting to address the Thomas comment on document how to enable those
>>>>>> config options in meson. @Thomas, can you please correct if I got it wrong?
>>>>> 
>>>>> If the doc is only about make, it will be removed in 2 months.
>>>>> So yes, I think it doesn't make sense to be merged as is.
>>>>> 
>>>> 
>>>> Make sense.
>>>> 
>>>> With meson, I see multiple ways to enable configuration options:
>>>> 
>>>> 1] macro in $(RTE_SDK)/config/rte_config.h
>>>> 2] using dpdk_conf.set() in $(RTE_SDK)/config/meson.build
>>>> 
>>>> I’d appreciate some suggestions on the best way to enable these options.
>>> 
>>> Neither 1 nor 2 because both are source files.
>>> Configuration must be done in the build directory.
>>> 
>>> We use "meson -D" to configure options listed in meson_options.txt
>>> The real question is which option we accept to be added.
>>> 
>>> 
>> 
>> Yes, correct. There are suggestions for making these as runtime configuration functions which can be enabled(or disabled) using ’set’ from within the application.
> 
> I don't understand this proposal.
> Is there a patch?
> 
>> Do you think, till this change is implemented, these options can be included in meson_options.txt?
> 
> In general, if you think there is a better alternative,
> please propose a patch.

I am not sure if changing it to a runtime function is a better alternative.
I am expecting some performance degradation with the change since the macros will be replaced with ‘if’ statements.
I will hack up a patch to see the performance difference and propose if there is not much of a difference.

> 
> I have no opinion what is best to manage such option.