diff mbox series

config: remove explicit undef of unset values

Message ID 20211216111430.699717-1-bruce.richardson@intel.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series config: remove explicit undef of unset values |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Bruce Richardson Dec. 16, 2021, 11:14 a.m. UTC 
  Rather than explicitly clearing any setting of undefined values in our
rte_config.h file, it's better to instead just add a comment that the
value is not set. Using a comment allows the user to set the value using
CFLAGS or similar mechanism without the config file clearing the value
again.

The text used "<VALUE> is not set" is modelled after the kernel approach
of doing the same thing.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---

Although DPDK coding convention forbids use of "//" for comments, using
regular C comment style makes the config settings less clear, as they can
be confused with regular comments in the file. Using "//" makes them stand
out better, so I prefer it. However, if others feel strongly, they can be
changed to standard.

Note: this is a resubmission of patch [1] which was part of a rejected
series. However, the reasons for rejection - values in config being out
of sync with those used for building apps - are less relevant for
many, if not all, of these setting, so I believe the benefits for
testing outweigh the potential downsides. If any setting is likely
problematic, I can keep the explicit undef for that case in a new patch
version.

[1] http://patches.dpdk.org/project/dpdk/patch/20200903144942.671870-2-bruce.richardson@intel.com/
---
 config/rte_config.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Bruce Richardson June 13, 2022, 9:54 a.m. UTC | #1 
On Thu, Dec 16, 2021 at 11:14:30AM +0000, Bruce Richardson wrote:
> Rather than explicitly clearing any setting of undefined values in our
> rte_config.h file, it's better to instead just add a comment that the
> value is not set. Using a comment allows the user to set the value using
> CFLAGS or similar mechanism without the config file clearing the value
> again.
> 
> The text used "<VALUE> is not set" is modelled after the kernel approach
> of doing the same thing.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> 
> Although DPDK coding convention forbids use of "//" for comments, using
> regular C comment style makes the config settings less clear, as they can
> be confused with regular comments in the file. Using "//" makes them stand
> out better, so I prefer it. However, if others feel strongly, they can be
> changed to standard.
> 
> Note: this is a resubmission of patch [1] which was part of a rejected
> series. However, the reasons for rejection - values in config being out
> of sync with those used for building apps - are less relevant for
> many, if not all, of these setting, so I believe the benefits for
> testing outweigh the potential downsides. If any setting is likely
> problematic, I can keep the explicit undef for that case in a new patch
> version.
> 
> [1] http://patches.dpdk.org/project/dpdk/patch/20200903144942.671870-2-bruce.richardson@intel.com/
> ---

Ping for review or feedback for this patch. I'd like to see it move forward
into a DPDK release if possible.

/Bruce

>  config/rte_config.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/config/rte_config.h b/config/rte_config.h
> index cab4390a97..953216babd 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -83,17 +83,17 @@
>  
>  /* ip_fragmentation defines */
>  #define RTE_LIBRTE_IP_FRAG_MAX_FRAG 8
> -#undef RTE_LIBRTE_IP_FRAG_TBL_STAT
> +// RTE_LIBRTE_IP_FRAG_TBL_STAT is not set
>  
>  /* rte_power defines */
>  #define RTE_MAX_LCORE_FREQS 64
>  
>  /* rte_sched defines */
> -#undef RTE_SCHED_CMAN
> -#undef RTE_SCHED_COLLECT_STATS
> -#undef RTE_SCHED_SUBPORT_TC_OV
> +// RTE_SCHED_CMAN is not set
> +// RTE_SCHED_COLLECT_STATS is not set
> +// RTE_SCHED_SUBPORT_TC_OV is not set
>  #define RTE_SCHED_PORT_N_GRINDERS 8
> -#undef RTE_SCHED_VECTOR
> +// RTE_SCHED_VECTOR is not set
>  
>  /* KNI defines */
>  #define RTE_KNI_PREEMPT_DEFAULT 1
> @@ -127,7 +127,7 @@
>  
>  /* i40e defines */
>  #define RTE_LIBRTE_I40E_RX_ALLOW_BULK_ALLOC 1
> -#undef RTE_LIBRTE_I40E_16BYTE_RX_DESC
> +// RTE_LIBRTE_I40E_16BYTE_RX_DESC is not set
>  #define RTE_LIBRTE_I40E_QUEUE_NUM_PER_PF 64
>  #define RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF 4
>  #define RTE_LIBRTE_I40E_QUEUE_NUM_PER_VM 4
> @@ -140,6 +140,6 @@
>  #define RTE_LIBRTE_QEDE_FW ""
>  
>  /* DLB2 defines */
> -#undef RTE_LIBRTE_PMD_DLB2_QUELL_STATS
> +// RTE_LIBRTE_PMD_DLB2_QUELL_STATS is not set
>  
>  #endif /* _RTE_CONFIG_H_ */
> -- 
> 2.32.0
>
Morten Brørup June 13, 2022, 10:01 a.m. UTC | #2 
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 13 June 2022 11.55
> To: dev@dpdk.org
> Cc: david.marchand@redhat.com; junx.dong@intel.com
> Subject: Re: [PATCH] config: remove explicit undef of unset values
> 
> On Thu, Dec 16, 2021 at 11:14:30AM +0000, Bruce Richardson wrote:
> > Rather than explicitly clearing any setting of undefined values in
> our
> > rte_config.h file, it's better to instead just add a comment that the
> > value is not set. Using a comment allows the user to set the value
> using
> > CFLAGS or similar mechanism without the config file clearing the
> value
> > again.
> >
> > The text used "<VALUE> is not set" is modelled after the kernel
> approach
> > of doing the same thing.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >
> > Although DPDK coding convention forbids use of "//" for comments,
> using
> > regular C comment style makes the config settings less clear, as they
> can
> > be confused with regular comments in the file. Using "//" makes them
> stand
> > out better, so I prefer it. However, if others feel strongly, they
> can be
> > changed to standard.
> >
> > Note: this is a resubmission of patch [1] which was part of a
> rejected
> > series. However, the reasons for rejection - values in config being
> out
> > of sync with those used for building apps - are less relevant for
> > many, if not all, of these setting, so I believe the benefits for
> > testing outweigh the potential downsides. If any setting is likely
> > problematic, I can keep the explicit undef for that case in a new
> patch
> > version.
> >
> > [1] http://patches.dpdk.org/project/dpdk/patch/20200903144942.671870-
> 2-bruce.richardson@intel.com/
> > ---
> 
> Ping for review or feedback for this patch. I'd like to see it move
> forward
> into a DPDK release if possible.
> 
> /Bruce
> 
> >  config/rte_config.h | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/config/rte_config.h b/config/rte_config.h
> > index cab4390a97..953216babd 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -83,17 +83,17 @@
> >
> >  /* ip_fragmentation defines */
> >  #define RTE_LIBRTE_IP_FRAG_MAX_FRAG 8
> > -#undef RTE_LIBRTE_IP_FRAG_TBL_STAT
> > +// RTE_LIBRTE_IP_FRAG_TBL_STAT is not set
> >

Yes, this is the right way to do it.

Acked-by: Morten Brørup <mb@smartsharesystems.com>
David Marchand June 13, 2022, 12:26 p.m. UTC | #3 
On Mon, Jun 13, 2022 at 11:54 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Thu, Dec 16, 2021 at 11:14:30AM +0000, Bruce Richardson wrote:
> > Rather than explicitly clearing any setting of undefined values in our
> > rte_config.h file, it's better to instead just add a comment that the
> > value is not set. Using a comment allows the user to set the value using
> > CFLAGS or similar mechanism without the config file clearing the value
> > again.
> >
> > The text used "<VALUE> is not set" is modelled after the kernel approach
> > of doing the same thing.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >
> > Although DPDK coding convention forbids use of "//" for comments, using
> > regular C comment style makes the config settings less clear, as they can
> > be confused with regular comments in the file. Using "//" makes them stand
> > out better, so I prefer it. However, if others feel strongly, they can be
> > changed to standard.
> >
> > Note: this is a resubmission of patch [1] which was part of a rejected
> > series. However, the reasons for rejection - values in config being out
> > of sync with those used for building apps - are less relevant for
> > many, if not all, of these setting, so I believe the benefits for
> > testing outweigh the potential downsides. If any setting is likely
> > problematic, I can keep the explicit undef for that case in a new patch
> > version.
> >
> > [1] http://patches.dpdk.org/project/dpdk/patch/20200903144942.671870-2-bruce.richardson@intel.com/
> > ---
>
> Ping for review or feedback for this patch. I'd like to see it move forward
> into a DPDK release if possible.

I'd like a check like (below), to avoid new additions:

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 34a2e43845..8dae47165e 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -158,6 +158,14 @@ check_forbidden_additions() { # <patch>
                -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
                "$1" || res=1

+       # '// XXX is not set' must be preferred over '#undef XXX'
+       awk -v FOLDERS='config/rte_config.h' \
+               -v EXPRESSIONS='#undef' \
+               -v RET_ON_FAIL=1 \
+               -v MESSAGE='Using "#undef XXX", prefer "// XXX is not set"' \
+               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+               "$1" || res=1
+
        return $res
 }

Otherwise, the change lgtm.
Bruce Richardson June 13, 2022, 12:37 p.m. UTC | #4 
On Mon, Jun 13, 2022 at 02:26:14PM +0200, David Marchand wrote:
> On Mon, Jun 13, 2022 at 11:54 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > On Thu, Dec 16, 2021 at 11:14:30AM +0000, Bruce Richardson wrote:
> > > Rather than explicitly clearing any setting of undefined values in our
> > > rte_config.h file, it's better to instead just add a comment that the
> > > value is not set. Using a comment allows the user to set the value using
> > > CFLAGS or similar mechanism without the config file clearing the value
> > > again.
> > >
> > > The text used "<VALUE> is not set" is modelled after the kernel approach
> > > of doing the same thing.
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >
> > > Although DPDK coding convention forbids use of "//" for comments, using
> > > regular C comment style makes the config settings less clear, as they can
> > > be confused with regular comments in the file. Using "//" makes them stand
> > > out better, so I prefer it. However, if others feel strongly, they can be
> > > changed to standard.
> > >
> > > Note: this is a resubmission of patch [1] which was part of a rejected
> > > series. However, the reasons for rejection - values in config being out
> > > of sync with those used for building apps - are less relevant for
> > > many, if not all, of these setting, so I believe the benefits for
> > > testing outweigh the potential downsides. If any setting is likely
> > > problematic, I can keep the explicit undef for that case in a new patch
> > > version.
> > >
> > > [1] http://patches.dpdk.org/project/dpdk/patch/20200903144942.671870-2-bruce.richardson@intel.com/
> > > ---
> >
> > Ping for review or feedback for this patch. I'd like to see it move forward
> > into a DPDK release if possible.
> 
> I'd like a check like (below), to avoid new additions:
> 
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 34a2e43845..8dae47165e 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -158,6 +158,14 @@ check_forbidden_additions() { # <patch>
>                 -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
>                 "$1" || res=1
> 
> +       # '// XXX is not set' must be preferred over '#undef XXX'
> +       awk -v FOLDERS='config/rte_config.h' \
> +               -v EXPRESSIONS='#undef' \
> +               -v RET_ON_FAIL=1 \
> +               -v MESSAGE='Using "#undef XXX", prefer "// XXX is not set"' \
> +               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> +               "$1" || res=1
> +
>         return $res
>  }
> 
> Otherwise, the change lgtm.
> 
Good idea. Do you want me to add your check above as a patch to this to
make a two-patch set for v2?

/Bruce
David Marchand June 13, 2022, 2:20 p.m. UTC | #5 
On Mon, Jun 13, 2022 at 2:37 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > I'd like a check like (below), to avoid new additions:
> >
> > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> > index 34a2e43845..8dae47165e 100755
> > --- a/devtools/checkpatches.sh
> > +++ b/devtools/checkpatches.sh
> > @@ -158,6 +158,14 @@ check_forbidden_additions() { # <patch>
> >                 -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> >                 "$1" || res=1
> >
> > +       # '// XXX is not set' must be preferred over '#undef XXX'
> > +       awk -v FOLDERS='config/rte_config.h' \
> > +               -v EXPRESSIONS='#undef' \
> > +               -v RET_ON_FAIL=1 \
> > +               -v MESSAGE='Using "#undef XXX", prefer "// XXX is not set"' \
> > +               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > +               "$1" || res=1
> > +
> >         return $res
> >  }
> >
> > Otherwise, the change lgtm.
> >
> Good idea. Do you want me to add your check above as a patch to this to
> make a two-patch set for v2?

If you are fine with the check, I don't mind it is part of this simple patch.
I can do it when applying if you are okay with it.
Tyler Retzlaff June 13, 2022, 2:46 p.m. UTC | #6 
On Mon, Jun 13, 2022 at 10:54:33AM +0100, Bruce Richardson wrote:
> On Thu, Dec 16, 2021 at 11:14:30AM +0000, Bruce Richardson wrote:
> > Rather than explicitly clearing any setting of undefined values in our
> > rte_config.h file, it's better to instead just add a comment that the
> > value is not set. Using a comment allows the user to set the value using
> > CFLAGS or similar mechanism without the config file clearing the value
> > again.
> > 
> > The text used "<VALUE> is not set" is modelled after the kernel approach
> > of doing the same thing.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > 
> > Although DPDK coding convention forbids use of "//" for comments, using
> > regular C comment style makes the config settings less clear, as they can
> > be confused with regular comments in the file. Using "//" makes them stand
> > out better, so I prefer it. However, if others feel strongly, they can be
> > changed to standard.
> > 
> > Note: this is a resubmission of patch [1] which was part of a rejected
> > series. However, the reasons for rejection - values in config being out
> > of sync with those used for building apps - are less relevant for
> > many, if not all, of these setting, so I believe the benefits for
> > testing outweigh the potential downsides. If any setting is likely
> > problematic, I can keep the explicit undef for that case in a new patch
> > version.
> > 
> > [1] http://patches.dpdk.org/project/dpdk/patch/20200903144942.671870-2-bruce.richardson@intel.com/

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Bruce Richardson June 13, 2022, 2:48 p.m. UTC | #7 
On Mon, Jun 13, 2022 at 04:20:04PM +0200, David Marchand wrote:
> On Mon, Jun 13, 2022 at 2:37 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > I'd like a check like (below), to avoid new additions:
> > >
> > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> > > index 34a2e43845..8dae47165e 100755
> > > --- a/devtools/checkpatches.sh
> > > +++ b/devtools/checkpatches.sh
> > > @@ -158,6 +158,14 @@ check_forbidden_additions() { # <patch>
> > >                 -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > >                 "$1" || res=1
> > >
> > > +       # '// XXX is not set' must be preferred over '#undef XXX'
> > > +       awk -v FOLDERS='config/rte_config.h' \
> > > +               -v EXPRESSIONS='#undef' \
> > > +               -v RET_ON_FAIL=1 \
> > > +               -v MESSAGE='Using "#undef XXX", prefer "// XXX is not set"' \
> > > +               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > > +               "$1" || res=1
> > > +
> > >         return $res
> > >  }
> > >
> > > Otherwise, the change lgtm.
> > >
> > Good idea. Do you want me to add your check above as a patch to this to
> > make a two-patch set for v2?
> 
> If you are fine with the check, I don't mind it is part of this simple patch.
> I can do it when applying if you are okay with it.
> 
Yes, I'm fine with it, thanks.
David Marchand June 15, 2022, 7:14 a.m. UTC | #8 
On Thu, Dec 16, 2021 at 12:15 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> Rather than explicitly clearing any setting of undefined values in our
> rte_config.h file, it's better to instead just add a comment that the
> value is not set. Using a comment allows the user to set the value using
> CFLAGS or similar mechanism without the config file clearing the value
> again.
>
> The text used "<VALUE> is not set" is modelled after the kernel approach
> of doing the same thing.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Rebased, added check as agreed, and applied.
Thanks Bruce.
diff mbox series

Patch

diff --git a/config/rte_config.h b/config/rte_config.h
index cab4390a97..953216babd 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -83,17 +83,17 @@ 
 
 /* ip_fragmentation defines */
 #define RTE_LIBRTE_IP_FRAG_MAX_FRAG 8
-#undef RTE_LIBRTE_IP_FRAG_TBL_STAT
+// RTE_LIBRTE_IP_FRAG_TBL_STAT is not set
 
 /* rte_power defines */
 #define RTE_MAX_LCORE_FREQS 64
 
 /* rte_sched defines */
-#undef RTE_SCHED_CMAN
-#undef RTE_SCHED_COLLECT_STATS
-#undef RTE_SCHED_SUBPORT_TC_OV
+// RTE_SCHED_CMAN is not set
+// RTE_SCHED_COLLECT_STATS is not set
+// RTE_SCHED_SUBPORT_TC_OV is not set
 #define RTE_SCHED_PORT_N_GRINDERS 8
-#undef RTE_SCHED_VECTOR
+// RTE_SCHED_VECTOR is not set
 
 /* KNI defines */
 #define RTE_KNI_PREEMPT_DEFAULT 1
@@ -127,7 +127,7 @@ 
 
 /* i40e defines */
 #define RTE_LIBRTE_I40E_RX_ALLOW_BULK_ALLOC 1
-#undef RTE_LIBRTE_I40E_16BYTE_RX_DESC
+// RTE_LIBRTE_I40E_16BYTE_RX_DESC is not set
 #define RTE_LIBRTE_I40E_QUEUE_NUM_PER_PF 64
 #define RTE_LIBRTE_I40E_QUEUE_NUM_PER_VF 4
 #define RTE_LIBRTE_I40E_QUEUE_NUM_PER_VM 4
@@ -140,6 +140,6 @@ 
 #define RTE_LIBRTE_QEDE_FW ""
 
 /* DLB2 defines */
-#undef RTE_LIBRTE_PMD_DLB2_QUELL_STATS
+// RTE_LIBRTE_PMD_DLB2_QUELL_STATS is not set
 
 #endif /* _RTE_CONFIG_H_ */