mbuf: document rule for new fields and flags

  Since dynamic fields and flags were added in 19.11,
the idea was to use them for new features, not only PMD-specific.

The rule is made more explicit in doxygen, in the mbuf guide,
and in the contribution design guidelines.

For more information about the original design, see the presentation
https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 doc/guides/contributing/design.rst | 13 +++++++++++++
 doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
 3 files changed, 38 insertions(+)
  

On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> Since dynamic fields and flags were added in 19.11,
> the idea was to use them for new features, not only PMD-specific.
>
> The rule is made more explicit in doxygen, in the mbuf guide,
> and in the contribution design guidelines.
>
> For more information about the original design, see the presentation
> https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  doc/guides/contributing/design.rst | 13 +++++++++++++
>  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
>  3 files changed, 38 insertions(+)
>
> diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> index d3dd694b65..508115d5bd 100644
> --- a/doc/guides/contributing/design.rst
> +++ b/doc/guides/contributing/design.rst
> @@ -57,6 +57,19 @@ The following config options can be used:
>  * ``CONFIG_RTE_EXEC_ENV`` is a string that contains the name of the executive environment.
>  * ``CONFIG_RTE_EXEC_ENV_FREEBSD`` or ``CONFIG_RTE_EXEC_ENV_LINUX`` are defined only if we are building for this execution environment.
>
> +Mbuf features
> +-------------
> +
> +The ``rte_mbuf`` structure must be kept small (128 bytes).
> +
> +In order to add new features without wasting buffer space for unused features,
> +some fields and flags can be registered dynamically in a shared area.

I think, instead of "can", it should be "must"

> +The "dynamic" mbuf area is the default choice for the new features.
> +
> +The "dynamic" area is eating the remaining space in mbuf,
> +and some existing "static" fields may need to become "dynamic".
> +
> +
>  Library Statistics
>  ------------------
>
> diff --git a/doc/guides/prog_guide/mbuf_lib.rst b/doc/guides/prog_guide/mbuf_lib.rst
> index 0d3223b081..c3dbfb9221 100644
> --- a/doc/guides/prog_guide/mbuf_lib.rst
> +++ b/doc/guides/prog_guide/mbuf_lib.rst
> @@ -207,6 +207,29 @@ The list of flags and their precise meaning is described in the mbuf API
>  documentation (rte_mbuf.h). Also refer to the testpmd source code
>  (specifically the csumonly.c file) for details.
>
> +Dynamic fields and flags
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The size of the mbuf is constrained and limited;
> +while the amount of metadata to save for each packet is quite unlimited.
> +The most basic networking information already find their place
> +in the existing mbuf fields and flags.
> +
> +If new features need to be added, the new fields and flags should fit

How about, instead of "should fit", must use the dynamic space.

> +in the "dynamic space", by registering some room in the mbuf structure:
> +
> +dynamic field
> +   named area in the mbuf structure,
> +   with a given size (at least 1 byte) and alignment constraint.
> +
> +dynamic flag
> +   named bit in the mbuf structure,
> +   stored in the field ``ol_flags``.
> +
> +The dynamic fields and flags are managed with the functions ``rte_mbuf_dyn*``.
> +
> +It is not possible to unregister fields or flags.
> +
>  .. _direct_indirect_buffer:
>
>  Direct and Indirect Buffers
> diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
> index b9a59c879c..22be41e520 100644
> --- a/lib/librte_mbuf/rte_mbuf_core.h
> +++ b/lib/librte_mbuf/rte_mbuf_core.h
> @@ -12,6 +12,8 @@
>   * packet offload flags and some related macros.
>   * For majority of DPDK entities, it is not recommended to include
>   * this file directly, use include <rte_mbuf.h> instead.
> + *
> + * New fields and flags should fit in the "dynamic space".

must use.

>   */
>
>  #include <stdint.h>
> --
> 2.26.2
>
  

Hi,

On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > Since dynamic fields and flags were added in 19.11,
> > the idea was to use them for new features, not only PMD-specific.
> >
> > The rule is made more explicit in doxygen, in the mbuf guide,
> > and in the contribution design guidelines.
> >
> > For more information about the original design, see the presentation
> > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  doc/guides/contributing/design.rst | 13 +++++++++++++
> >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> >  3 files changed, 38 insertions(+)
> >
> > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > index d3dd694b65..508115d5bd 100644
> > --- a/doc/guides/contributing/design.rst
> > +++ b/doc/guides/contributing/design.rst
> > @@ -57,6 +57,19 @@ The following config options can be used:
> >  * ``CONFIG_RTE_EXEC_ENV`` is a string that contains the name of the executive environment.
> >  * ``CONFIG_RTE_EXEC_ENV_FREEBSD`` or ``CONFIG_RTE_EXEC_ENV_LINUX`` are defined only if we are building for this execution environment.
> >
> > +Mbuf features
> > +-------------
> > +
> > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > +
> > +In order to add new features without wasting buffer space for unused features,
> > +some fields and flags can be registered dynamically in a shared area.
> 
> I think, instead of "can", it should be "must"
>
> > +The "dynamic" mbuf area is the default choice for the new features.

In my opinion, Thomas' proposal is correct, with the next sentence
saying it is the default choice for new features.

Giving guidelines is a good thing (thanks Thomas for documenting it),
but I don't think we should be too strict: the door remains open for
technical debate and exceptions.


> > +
> > +The "dynamic" area is eating the remaining space in mbuf,
> > +and some existing "static" fields may need to become "dynamic".
> > +
> > +
> >  Library Statistics
> >  ------------------
> >
> > diff --git a/doc/guides/prog_guide/mbuf_lib.rst b/doc/guides/prog_guide/mbuf_lib.rst
> > index 0d3223b081..c3dbfb9221 100644
> > --- a/doc/guides/prog_guide/mbuf_lib.rst
> > +++ b/doc/guides/prog_guide/mbuf_lib.rst
> > @@ -207,6 +207,29 @@ The list of flags and their precise meaning is described in the mbuf API
> >  documentation (rte_mbuf.h). Also refer to the testpmd source code
> >  (specifically the csumonly.c file) for details.
> >
> > +Dynamic fields and flags
> > +~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +The size of the mbuf is constrained and limited;
> > +while the amount of metadata to save for each packet is quite unlimited.
> > +The most basic networking information already find their place
> > +in the existing mbuf fields and flags.
> > +
> > +If new features need to be added, the new fields and flags should fit
> 
> How about, instead of "should fit", must use the dynamic space.

Same comment here, I think Thomas's original sentence is fine.

> 
> > +in the "dynamic space", by registering some room in the mbuf structure:
> > +
> > +dynamic field
> > +   named area in the mbuf structure,
> > +   with a given size (at least 1 byte) and alignment constraint.
> > +
> > +dynamic flag
> > +   named bit in the mbuf structure,
> > +   stored in the field ``ol_flags``.
> > +
> > +The dynamic fields and flags are managed with the functions ``rte_mbuf_dyn*``.
> > +
> > +It is not possible to unregister fields or flags.
> > +
> >  .. _direct_indirect_buffer:
> >
> >  Direct and Indirect Buffers
> > diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
> > index b9a59c879c..22be41e520 100644
> > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > @@ -12,6 +12,8 @@
> >   * packet offload flags and some related macros.
> >   * For majority of DPDK entities, it is not recommended to include
> >   * this file directly, use include <rte_mbuf.h> instead.
> > + *
> > + * New fields and flags should fit in the "dynamic space".
> 
> must use.

Same comment here.


Acked-by: Olivier Matz <olivier.matz@6wind.com>
  

On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> Hi,
>
> On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > Since dynamic fields and flags were added in 19.11,
> > > the idea was to use them for new features, not only PMD-specific.
> > >
> > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > and in the contribution design guidelines.
> > >
> > > For more information about the original design, see the presentation
> > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > ---
> > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > >  3 files changed, 38 insertions(+)
> > >
> > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > index d3dd694b65..508115d5bd 100644
> > > --- a/doc/guides/contributing/design.rst
> > > +++ b/doc/guides/contributing/design.rst
> > > @@ -57,6 +57,19 @@ The following config options can be used:
> > >  * ``CONFIG_RTE_EXEC_ENV`` is a string that contains the name of the executive environment.
> > >  * ``CONFIG_RTE_EXEC_ENV_FREEBSD`` or ``CONFIG_RTE_EXEC_ENV_LINUX`` are defined only if we are building for this execution environment.
> > >
> > > +Mbuf features
> > > +-------------
> > > +
> > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > +
> > > +In order to add new features without wasting buffer space for unused features,
> > > +some fields and flags can be registered dynamically in a shared area.
> >
> > I think, instead of "can", it should be "must"
> >
> > > +The "dynamic" mbuf area is the default choice for the new features.
>
> In my opinion, Thomas' proposal is correct, with the next sentence
> saying it is the default choice for new features.
>
> Giving guidelines is a good thing (thanks Thomas for documenting it),
> but I don't think we should be too strict: the door remains open for
> technical debate and exceptions.

If you are open for the exception then it must be mention in what case
the exception is allowed and what are the criteria of the exception?

For example,  Why did n't we choose the following patch as expectation
http://patches.dpdk.org/patch/68733/  even if only one bit used.

If we are not not defining the criteria, IMO, This patch serve no purpose than
the existing situation.

Do you think, any case where the dynamic scheme can not be used as a replacement
for static other than performance hit.
  

On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> >
> > Hi,
> >
> > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > >
> > > > Since dynamic fields and flags were added in 19.11,
> > > > the idea was to use them for new features, not only PMD-specific.
> > > >
> > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > and in the contribution design guidelines.
> > > >
> > > > For more information about the original design, see the presentation
> > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > ---
> > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > >  3 files changed, 38 insertions(+)
> > > >
> > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > index d3dd694b65..508115d5bd 100644
> > > > --- a/doc/guides/contributing/design.rst
> > > > +++ b/doc/guides/contributing/design.rst
> > > > @@ -57,6 +57,19 @@ The following config options can be used:
> > > >  * ``CONFIG_RTE_EXEC_ENV`` is a string that contains the name of the executive environment.
> > > >  * ``CONFIG_RTE_EXEC_ENV_FREEBSD`` or ``CONFIG_RTE_EXEC_ENV_LINUX`` are defined only if we are building for this execution environment.
> > > >
> > > > +Mbuf features
> > > > +-------------
> > > > +
> > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > +
> > > > +In order to add new features without wasting buffer space for unused features,
> > > > +some fields and flags can be registered dynamically in a shared area.
> > >
> > > I think, instead of "can", it should be "must"
> > >
> > > > +The "dynamic" mbuf area is the default choice for the new features.
> >
> > In my opinion, Thomas' proposal is correct, with the next sentence
> > saying it is the default choice for new features.
> >
> > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > but I don't think we should be too strict: the door remains open for
> > technical debate and exceptions.
> 
> If you are open for the exception then it must be mention in what case
> the exception is allowed and what are the criteria of the exception?
> 
> For example,  Why did n't we choose the following patch as expectation
> http://patches.dpdk.org/patch/68733/  even if only one bit used.
> 
> If we are not not defining the criteria, IMO, This patch serve no purpose than
> the existing situation.
> 
> Do you think, any case where the dynamic scheme can not be used as a replacement
> for static other than performance hit.

I don't think it is possible to anticipate all criteria in the
documentation. With Thomas' proposal, it gives a direction is and a
global view, but it must not completly replace reflection and
discussion.

Olivier
  

On Wed, May 27, 2020 at 12:39 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> > On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > >
> > > > > Since dynamic fields and flags were added in 19.11,
> > > > > the idea was to use them for new features, not only PMD-specific.
> > > > >
> > > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > > and in the contribution design guidelines.
> > > > >
> > > > > For more information about the original design, see the presentation
> > > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > > >
> > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > ---
> > > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > > >  3 files changed, 38 insertions(+)
> > > > >
> > > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > > index d3dd694b65..508115d5bd 100644
> > > > > --- a/doc/guides/contributing/design.rst
> > > > > +++ b/doc/guides/contributing/design.rst
> > > > > @@ -57,6 +57,19 @@ The following config options can be used:
> > > > >  * ``CONFIG_RTE_EXEC_ENV`` is a string that contains the name of the executive environment.
> > > > >  * ``CONFIG_RTE_EXEC_ENV_FREEBSD`` or ``CONFIG_RTE_EXEC_ENV_LINUX`` are defined only if we are building for this execution environment.
> > > > >
> > > > > +Mbuf features
> > > > > +-------------
> > > > > +
> > > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > > +
> > > > > +In order to add new features without wasting buffer space for unused features,
> > > > > +some fields and flags can be registered dynamically in a shared area.
> > > >
> > > > I think, instead of "can", it should be "must"
> > > >
> > > > > +The "dynamic" mbuf area is the default choice for the new features.
> > >
> > > In my opinion, Thomas' proposal is correct, with the next sentence
> > > saying it is the default choice for new features.
> > >
> > > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > > but I don't think we should be too strict: the door remains open for
> > > technical debate and exceptions.
> >
> > If you are open for the exception then it must be mention in what case
> > the exception is allowed and what are the criteria of the exception?
> >
> > For example,  Why did n't we choose the following patch as expectation
> > http://patches.dpdk.org/patch/68733/  even if only one bit used.
> >
> > If we are not not defining the criteria, IMO, This patch serve no purpose than
> > the existing situation.
> >
> > Do you think, any case where the dynamic scheme can not be used as a replacement
> > for static other than performance hit.
>
> I don't think it is possible to anticipate all criteria in the
> documentation. With Thomas' proposal, it gives a direction is and a
> global view, but it must not completly replace reflection and
> discussion.

I don't think, we need to anticipate all the criteria in the documentation.
At least ONE should be given as an example of an exception.
I would say,
a) If a feature takes only one bit and its part of normative API spec
and it used in fastpath we should consider the static scheme.
b) Adding an exception to the existing list needs approval at least
from three maintainers

For me, it is a very legitimate case to have support for
http://patches.dpdk.org/patch/68733/ to the static scheme
as it takes 1 bit for a feature and it is part of the normative spec.
I don't get in explanation in the ml, why
we can not make it as the static scheme for this case.

My worry is, if we are keeping as open-ended means, we are giving room
for the disparity among the vendors/feature
as I dont think, There is use case where dynamic scheme can not be
used as a replacement
for static other than performance hit.(Could think of any use case?)
So open ended boils down to preference to specific feature/vendor. I
think,that path should be avoided.









>
> Olivier
  

27/05/2020 09:31, Jerin Jacob:
> On Wed, May 27, 2020 at 12:39 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> > > On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > >
> > > > > > Since dynamic fields and flags were added in 19.11,
> > > > > > the idea was to use them for new features, not only PMD-specific.
> > > > > >
> > > > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > > > and in the contribution design guidelines.
> > > > > >
> > > > > > For more information about the original design, see the presentation
> > > > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > > > >
> > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > > ---
> > > > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > > > >  3 files changed, 38 insertions(+)
> > > > > >
> > > > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > > > index d3dd694b65..508115d5bd 100644
> > > > > > --- a/doc/guides/contributing/design.rst
> > > > > > +++ b/doc/guides/contributing/design.rst
> > > > > > +Mbuf features
> > > > > > +-------------
> > > > > > +
> > > > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > > > +
> > > > > > +In order to add new features without wasting buffer space for unused features,
> > > > > > +some fields and flags can be registered dynamically in a shared area.
> > > > >
> > > > > I think, instead of "can", it should be "must"
> > > > >
> > > > > > +The "dynamic" mbuf area is the default choice for the new features.
> > > >
> > > > In my opinion, Thomas' proposal is correct, with the next sentence
> > > > saying it is the default choice for new features.
> > > >
> > > > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > > > but I don't think we should be too strict: the door remains open for
> > > > technical debate and exceptions.
> > >
> > > If you are open for the exception then it must be mention in what case
> > > the exception is allowed and what are the criteria of the exception?
> > >
> > > For example,  Why did n't we choose the following patch as expectation
> > > http://patches.dpdk.org/patch/68733/  even if only one bit used.
> > >
> > > If we are not not defining the criteria, IMO, This patch serve no purpose than
> > > the existing situation.
> > >
> > > Do you think, any case where the dynamic scheme can not be used as a replacement
> > > for static other than performance hit.
> >
> > I don't think it is possible to anticipate all criteria in the
> > documentation. With Thomas' proposal, it gives a direction is and a
> > global view, but it must not completly replace reflection and
> > discussion.
> 
> I don't think, we need to anticipate all the criteria in the documentation.
> At least ONE should be given as an example of an exception.

I think it is too early to be more specific in the guidelines.
Do we agree this patch is a first good step in the documentation?
We can extend the guidelines a bit later after having some
discussions on specific cases, and probably in the techboard too.


> I would say,
> a) If a feature takes only one bit and its part of normative API spec
> and it used in fastpath we should consider the static scheme.
> b) Adding an exception to the existing list needs approval at least
> from three maintainers
> 
> For me, it is a very legitimate case to have support for
> http://patches.dpdk.org/patch/68733/ to the static scheme
> as it takes 1 bit for a feature and it is part of the normative spec.
> I don't get in explanation in the ml, why
> we can not make it as the static scheme for this case.

We can continue discussion about this specific case in the right thread.
Note: I don't have a definitive opinion on it, I need to read it carefully.


> My worry is, if we are keeping as open-ended means, we are giving room
> for the disparity among the vendors/feature
> as I dont think, There is use case where dynamic scheme can not be
> used as a replacement
> for static other than performance hit.(Could think of any use case?)
> So open ended boils down to preference to specific feature/vendor. I
> think,that path should be avoided.

Of course all rules and decisions have to be fair.
It's not even a question.
  

On Wed, May 27, 2020 at 3:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 27/05/2020 09:31, Jerin Jacob:
> > On Wed, May 27, 2020 at 12:39 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> > > > On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > > > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > >
> > > > > > > Since dynamic fields and flags were added in 19.11,
> > > > > > > the idea was to use them for new features, not only PMD-specific.
> > > > > > >
> > > > > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > > > > and in the contribution design guidelines.
> > > > > > >
> > > > > > > For more information about the original design, see the presentation
> > > > > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > > > > >
> > > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > ---
> > > > > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > > > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > > > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > > > > >  3 files changed, 38 insertions(+)
> > > > > > >
> > > > > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > > > > index d3dd694b65..508115d5bd 100644
> > > > > > > --- a/doc/guides/contributing/design.rst
> > > > > > > +++ b/doc/guides/contributing/design.rst
> > > > > > > +Mbuf features
> > > > > > > +-------------
> > > > > > > +
> > > > > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > > > > +
> > > > > > > +In order to add new features without wasting buffer space for unused features,
> > > > > > > +some fields and flags can be registered dynamically in a shared area.
> > > > > >
> > > > > > I think, instead of "can", it should be "must"
> > > > > >
> > > > > > > +The "dynamic" mbuf area is the default choice for the new features.
> > > > >
> > > > > In my opinion, Thomas' proposal is correct, with the next sentence
> > > > > saying it is the default choice for new features.
> > > > >
> > > > > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > > > > but I don't think we should be too strict: the door remains open for
> > > > > technical debate and exceptions.
> > > >
> > > > If you are open for the exception then it must be mention in what case
> > > > the exception is allowed and what are the criteria of the exception?
> > > >
> > > > For example,  Why did n't we choose the following patch as expectation
> > > > http://patches.dpdk.org/patch/68733/  even if only one bit used.
> > > >
> > > > If we are not not defining the criteria, IMO, This patch serve no purpose than
> > > > the existing situation.
> > > >
> > > > Do you think, any case where the dynamic scheme can not be used as a replacement
> > > > for static other than performance hit.
> > >
> > > I don't think it is possible to anticipate all criteria in the
> > > documentation. With Thomas' proposal, it gives a direction is and a
> > > global view, but it must not completly replace reflection and
> > > discussion.
> >
> > I don't think, we need to anticipate all the criteria in the documentation.
> > At least ONE should be given as an example of an exception.
>
> I think it is too early to be more specific in the guidelines.
> Do we agree this patch is a first good step in the documentation?

IMO, there is a gap. The subject says the rule, but no rule here.
We are just giving some guideline and following info in the patch
given by Olivier is not
expressed if we read the patch.

"
 I don't think we should be too strict: the door remains open for
technical debate and exceptions.
"

> We can extend the guidelines a bit later after having some
> discussions on specific cases, and probably in the techboard too.
>
>
> > I would say,
> > a) If a feature takes only one bit and its part of normative API spec
> > and it used in fastpath we should consider the static scheme.
> > b) Adding an exception to the existing list needs approval at least
> > from three maintainers
> >
> > For me, it is a very legitimate case to have support for
> > http://patches.dpdk.org/patch/68733/ to the static scheme
> > as it takes 1 bit for a feature and it is part of the normative spec.
> > I don't get in explanation in the ml, why
> > we can not make it as the static scheme for this case.
>
> We can continue discussion about this specific case in the right thread.

Yes. The email thread[1] provided all the details. We have optimized
to one bit for this feature.
We are expecting Olivier to comment on the new proposal.
[1]
http://patches.dpdk.org/patch/68733/

> Note: I don't have a definitive opinion on it, I need to read it carefully.

Please read it carefully and please provide any technical opinions if
you have any.


>
>
> > My worry is, if we are keeping as open-ended means, we are giving room
> > for the disparity among the vendors/feature
> > as I dont think, There is use case where dynamic scheme can not be
> > used as a replacement
> > for static other than performance hit.(Could think of any use case?)
> > So open ended boils down to preference to specific feature/vendor. I
> > think,that path should be avoided.
>
> Of course all rules and decisions have to be fair.
> It's not even a question.

Yes. But I dont think, this patch is not  enforcing anything such,
instead it makes it as an open-ended
for more confusion. IMO, if it not black and white then better to not
express the rule.




>
>
  

27/05/2020 13:43, Jerin Jacob:
> On Wed, May 27, 2020 at 3:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 27/05/2020 09:31, Jerin Jacob:
> > > On Wed, May 27, 2020 at 12:39 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> > > > > On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > > > > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > > >
> > > > > > > > Since dynamic fields and flags were added in 19.11,
> > > > > > > > the idea was to use them for new features, not only PMD-specific.
> > > > > > > >
> > > > > > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > > > > > and in the contribution design guidelines.
> > > > > > > >
> > > > > > > > For more information about the original design, see the presentation
> > > > > > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > > > > > >
> > > > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > ---
> > > > > > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > > > > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > > > > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > > > > > >  3 files changed, 38 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > > > > > index d3dd694b65..508115d5bd 100644
> > > > > > > > --- a/doc/guides/contributing/design.rst
> > > > > > > > +++ b/doc/guides/contributing/design.rst
> > > > > > > > +Mbuf features
> > > > > > > > +-------------
> > > > > > > > +
> > > > > > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > > > > > +
> > > > > > > > +In order to add new features without wasting buffer space for unused features,
> > > > > > > > +some fields and flags can be registered dynamically in a shared area.
> > > > > > >
> > > > > > > I think, instead of "can", it should be "must"
> > > > > > >
> > > > > > > > +The "dynamic" mbuf area is the default choice for the new features.
> > > > > >
> > > > > > In my opinion, Thomas' proposal is correct, with the next sentence
> > > > > > saying it is the default choice for new features.
> > > > > >
> > > > > > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > > > > > but I don't think we should be too strict: the door remains open for
> > > > > > technical debate and exceptions.
> > > > >
> > > > > If you are open for the exception then it must be mention in what case
> > > > > the exception is allowed and what are the criteria of the exception?
> > > > >
> > > > > For example,  Why did n't we choose the following patch as expectation
> > > > > http://patches.dpdk.org/patch/68733/  even if only one bit used.
> > > > >
> > > > > If we are not not defining the criteria, IMO, This patch serve no purpose than
> > > > > the existing situation.
> > > > >
> > > > > Do you think, any case where the dynamic scheme can not be used as a replacement
> > > > > for static other than performance hit.
> > > >
> > > > I don't think it is possible to anticipate all criteria in the
> > > > documentation. With Thomas' proposal, it gives a direction is and a
> > > > global view, but it must not completly replace reflection and
> > > > discussion.
> > >
> > > I don't think, we need to anticipate all the criteria in the documentation.
> > > At least ONE should be given as an example of an exception.
> >
> > I think it is too early to be more specific in the guidelines.
> > Do we agree this patch is a first good step in the documentation?
> 
> IMO, there is a gap. The subject says the rule, but no rule here.
> We are just giving some guideline and following info in the patch
> given by Olivier is not
> expressed if we read the patch.
> 
> "
>  I don't think we should be too strict: the door remains open for
> technical debate and exceptions.
> "

Indeed, the headline should be
	mbuf: add guideline for new fields and flags


> > We can extend the guidelines a bit later after having some
> > discussions on specific cases, and probably in the techboard too.
> >
> >
> > > I would say,
> > > a) If a feature takes only one bit and its part of normative API spec
> > > and it used in fastpath we should consider the static scheme.
> > > b) Adding an exception to the existing list needs approval at least
> > > from three maintainers
> > >
> > > For me, it is a very legitimate case to have support for
> > > http://patches.dpdk.org/patch/68733/ to the static scheme
> > > as it takes 1 bit for a feature and it is part of the normative spec.
> > > I don't get in explanation in the ml, why
> > > we can not make it as the static scheme for this case.
> >
> > We can continue discussion about this specific case in the right thread.
> 
> Yes. The email thread[1] provided all the details. We have optimized
> to one bit for this feature.
> We are expecting Olivier to comment on the new proposal.
> [1]
> http://patches.dpdk.org/patch/68733/
> 
> > Note: I don't have a definitive opinion on it, I need to read it carefully.
> 
> Please read it carefully and please provide any technical opinions if
> you have any.
> 
> 
> > > My worry is, if we are keeping as open-ended means, we are giving room
> > > for the disparity among the vendors/feature
> > > as I dont think, There is use case where dynamic scheme can not be
> > > used as a replacement
> > > for static other than performance hit.(Could think of any use case?)
> > > So open ended boils down to preference to specific feature/vendor. I
> > > think,that path should be avoided.
> >
> > Of course all rules and decisions have to be fair.
> > It's not even a question.
> 
> Yes. But I dont think, this patch is not  enforcing anything such,
> instead it makes it as an open-ended
> for more confusion. IMO, if it not black and white then better to not
> express the rule.

I disagree about "more confusion".
I think the value of this patch is to improve awareness
about the need for using dynamic fields and flags.

Let's ask other opinions about the added value of this patch.
  

On Wed, May 27, 2020 at 5:26 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 27/05/2020 13:43, Jerin Jacob:
> > On Wed, May 27, 2020 at 3:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 27/05/2020 09:31, Jerin Jacob:
> > > > On Wed, May 27, 2020 at 12:39 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> > > > > > On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > > > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > > > > > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > > > >
> > > > > > > > > Since dynamic fields and flags were added in 19.11,
> > > > > > > > > the idea was to use them for new features, not only PMD-specific.
> > > > > > > > >
> > > > > > > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > > > > > > and in the contribution design guidelines.
> > > > > > > > >
> > > > > > > > > For more information about the original design, see the presentation
> > > > > > > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > > > > > > >
> > > > > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > > ---
> > > > > > > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > > > > > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > > > > > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > > > > > > >  3 files changed, 38 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > > > > > > index d3dd694b65..508115d5bd 100644
> > > > > > > > > --- a/doc/guides/contributing/design.rst
> > > > > > > > > +++ b/doc/guides/contributing/design.rst
> > > > > > > > > +Mbuf features
> > > > > > > > > +-------------
> > > > > > > > > +
> > > > > > > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > > > > > > +
> > > > > > > > > +In order to add new features without wasting buffer space for unused features,
> > > > > > > > > +some fields and flags can be registered dynamically in a shared area.
> > > > > > > >
> > > > > > > > I think, instead of "can", it should be "must"
> > > > > > > >
> > > > > > > > > +The "dynamic" mbuf area is the default choice for the new features.
> > > > > > >
> > > > > > > In my opinion, Thomas' proposal is correct, with the next sentence
> > > > > > > saying it is the default choice for new features.
> > > > > > >
> > > > > > > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > > > > > > but I don't think we should be too strict: the door remains open for
> > > > > > > technical debate and exceptions.
> > > > > >
> > > > > > If you are open for the exception then it must be mention in what case
> > > > > > the exception is allowed and what are the criteria of the exception?
> > > > > >
> > > > > > For example,  Why did n't we choose the following patch as expectation
> > > > > > http://patches.dpdk.org/patch/68733/  even if only one bit used.
> > > > > >
> > > > > > If we are not not defining the criteria, IMO, This patch serve no purpose than
> > > > > > the existing situation.
> > > > > >
> > > > > > Do you think, any case where the dynamic scheme can not be used as a replacement
> > > > > > for static other than performance hit.
> > > > >
> > > > > I don't think it is possible to anticipate all criteria in the
> > > > > documentation. With Thomas' proposal, it gives a direction is and a
> > > > > global view, but it must not completly replace reflection and
> > > > > discussion.
> > > >
> > > > I don't think, we need to anticipate all the criteria in the documentation.
> > > > At least ONE should be given as an example of an exception.
> > >
> > > I think it is too early to be more specific in the guidelines.
> > > Do we agree this patch is a first good step in the documentation?
> >
> > IMO, there is a gap. The subject says the rule, but no rule here.
> > We are just giving some guideline and following info in the patch
> > given by Olivier is not
> > expressed if we read the patch.
> >
> > "
> >  I don't think we should be too strict: the door remains open for
> > technical debate and exceptions.
> > "
>
> Indeed, the headline should be
>         mbuf: add guideline for new fields and flags
>
>
> > > We can extend the guidelines a bit later after having some
> > > discussions on specific cases, and probably in the techboard too.
> > >
> > >
> > > > I would say,
> > > > a) If a feature takes only one bit and its part of normative API spec
> > > > and it used in fastpath we should consider the static scheme.
> > > > b) Adding an exception to the existing list needs approval at least
> > > > from three maintainers
> > > >
> > > > For me, it is a very legitimate case to have support for
> > > > http://patches.dpdk.org/patch/68733/ to the static scheme
> > > > as it takes 1 bit for a feature and it is part of the normative spec.
> > > > I don't get in explanation in the ml, why
> > > > we can not make it as the static scheme for this case.
> > >
> > > We can continue discussion about this specific case in the right thread.
> >
> > Yes. The email thread[1] provided all the details. We have optimized
> > to one bit for this feature.
> > We are expecting Olivier to comment on the new proposal.
> > [1]
> > http://patches.dpdk.org/patch/68733/
> >
> > > Note: I don't have a definitive opinion on it, I need to read it carefully.
> >
> > Please read it carefully and please provide any technical opinions if
> > you have any.
> >
> >
> > > > My worry is, if we are keeping as open-ended means, we are giving room
> > > > for the disparity among the vendors/feature
> > > > as I dont think, There is use case where dynamic scheme can not be
> > > > used as a replacement
> > > > for static other than performance hit.(Could think of any use case?)
> > > > So open ended boils down to preference to specific feature/vendor. I
> > > > think,that path should be avoided.
> > >
> > > Of course all rules and decisions have to be fair.
> > > It's not even a question.
> >
> > Yes. But I dont think, this patch is not  enforcing anything such,
> > instead it makes it as an open-ended
> > for more confusion. IMO, if it not black and white then better to not
> > express the rule.
>
> I disagree about "more confusion".

My confusion will clear up if
1) s/rule/guide line/ change across the patch
2) Explicit mention of the following or similar sentence.
it is a guideline and exception is allowed on a case by case using
technical debate.

> I think the value of this patch is to improve awareness
> about the need for using dynamic fields and flags.
>
> Let's ask other opinions about the added value of this patch.
>
>
  

On Wed, May 27, 2020 at 05:37:13PM +0530, Jerin Jacob wrote:
> On Wed, May 27, 2020 at 5:26 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 27/05/2020 13:43, Jerin Jacob:
> > > On Wed, May 27, 2020 at 3:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > 27/05/2020 09:31, Jerin Jacob:
> > > > > On Wed, May 27, 2020 at 12:39 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > > On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> > > > > > > On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > > > > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > > > > > > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > > > > >
> > > > > > > > > > Since dynamic fields and flags were added in 19.11,
> > > > > > > > > > the idea was to use them for new features, not only PMD-specific.
> > > > > > > > > >
> > > > > > > > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > > > > > > > and in the contribution design guidelines.
> > > > > > > > > >
> > > > > > > > > > For more information about the original design, see the presentation
> > > > > > > > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > > > ---
> > > > > > > > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > > > > > > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > > > > > > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > > > > > > > >  3 files changed, 38 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > > > > > > > index d3dd694b65..508115d5bd 100644
> > > > > > > > > > --- a/doc/guides/contributing/design.rst
> > > > > > > > > > +++ b/doc/guides/contributing/design.rst
> > > > > > > > > > +Mbuf features
> > > > > > > > > > +-------------
> > > > > > > > > > +
> > > > > > > > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > > > > > > > +
> > > > > > > > > > +In order to add new features without wasting buffer space for unused features,
> > > > > > > > > > +some fields and flags can be registered dynamically in a shared area.
> > > > > > > > >
> > > > > > > > > I think, instead of "can", it should be "must"
> > > > > > > > >
> > > > > > > > > > +The "dynamic" mbuf area is the default choice for the new features.
> > > > > > > >
> > > > > > > > In my opinion, Thomas' proposal is correct, with the next sentence
> > > > > > > > saying it is the default choice for new features.
> > > > > > > >
> > > > > > > > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > > > > > > > but I don't think we should be too strict: the door remains open for
> > > > > > > > technical debate and exceptions.
> > > > > > >
> > > > > > > If you are open for the exception then it must be mention in what case
> > > > > > > the exception is allowed and what are the criteria of the exception?
> > > > > > >
> > > > > > > For example,  Why did n't we choose the following patch as expectation
> > > > > > > http://patches.dpdk.org/patch/68733/  even if only one bit used.
> > > > > > >
> > > > > > > If we are not not defining the criteria, IMO, This patch serve no purpose than
> > > > > > > the existing situation.
> > > > > > >
> > > > > > > Do you think, any case where the dynamic scheme can not be used as a replacement
> > > > > > > for static other than performance hit.
> > > > > >
> > > > > > I don't think it is possible to anticipate all criteria in the
> > > > > > documentation. With Thomas' proposal, it gives a direction is and a
> > > > > > global view, but it must not completly replace reflection and
> > > > > > discussion.
> > > > >
> > > > > I don't think, we need to anticipate all the criteria in the documentation.
> > > > > At least ONE should be given as an example of an exception.
> > > >
> > > > I think it is too early to be more specific in the guidelines.
> > > > Do we agree this patch is a first good step in the documentation?
> > >
> > > IMO, there is a gap. The subject says the rule, but no rule here.
> > > We are just giving some guideline and following info in the patch
> > > given by Olivier is not
> > > expressed if we read the patch.
> > >
> > > "
> > >  I don't think we should be too strict: the door remains open for
> > > technical debate and exceptions.
> > > "
> >
> > Indeed, the headline should be
> >         mbuf: add guideline for new fields and flags
> >
> >
> > > > We can extend the guidelines a bit later after having some
> > > > discussions on specific cases, and probably in the techboard too.
> > > >
> > > >
> > > > > I would say,
> > > > > a) If a feature takes only one bit and its part of normative API spec
> > > > > and it used in fastpath we should consider the static scheme.
> > > > > b) Adding an exception to the existing list needs approval at least
> > > > > from three maintainers
> > > > >
> > > > > For me, it is a very legitimate case to have support for
> > > > > http://patches.dpdk.org/patch/68733/ to the static scheme
> > > > > as it takes 1 bit for a feature and it is part of the normative spec.
> > > > > I don't get in explanation in the ml, why
> > > > > we can not make it as the static scheme for this case.
> > > >
> > > > We can continue discussion about this specific case in the right thread.
> > >
> > > Yes. The email thread[1] provided all the details. We have optimized
> > > to one bit for this feature.
> > > We are expecting Olivier to comment on the new proposal.
> > > [1]
> > > http://patches.dpdk.org/patch/68733/
> > >
> > > > Note: I don't have a definitive opinion on it, I need to read it carefully.
> > >
> > > Please read it carefully and please provide any technical opinions if
> > > you have any.
> > >
> > >
> > > > > My worry is, if we are keeping as open-ended means, we are giving room
> > > > > for the disparity among the vendors/feature
> > > > > as I dont think, There is use case where dynamic scheme can not be
> > > > > used as a replacement
> > > > > for static other than performance hit.(Could think of any use case?)
> > > > > So open ended boils down to preference to specific feature/vendor. I
> > > > > think,that path should be avoided.
> > > >
> > > > Of course all rules and decisions have to be fair.
> > > > It's not even a question.
> > >
> > > Yes. But I dont think, this patch is not  enforcing anything such,
> > > instead it makes it as an open-ended
> > > for more confusion. IMO, if it not black and white then better to not
> > > express the rule.
> >
> > I disagree about "more confusion".
> 
> My confusion will clear up if
> 1) s/rule/guide line/ change across the patch

OK for me, guideline is indeed a better name.

> 2) Explicit mention of the following or similar sentence.
> it is a guideline and exception is allowed on a case by case using
> technical debate.

In my opinion, anything written in the documentation is questionable and
discussable (because the code or the environment can change, or because someone
can come with new arguments), and I don't really see the added value to
explicitly say it... or we'll have to say it for every guideline in the
documentation.


> 
> > I think the value of this patch is to improve awareness
> > about the need for using dynamic fields and flags.
> >
> > Let's ask other opinions about the added value of this patch.
> >
> >
  

On Wed, May 27, 2020 at 5:53 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Wed, May 27, 2020 at 05:37:13PM +0530, Jerin Jacob wrote:
> > On Wed, May 27, 2020 at 5:26 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 27/05/2020 13:43, Jerin Jacob:
> > > > On Wed, May 27, 2020 at 3:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > 27/05/2020 09:31, Jerin Jacob:
> > > > > > On Wed, May 27, 2020 at 12:39 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > > > On Tue, May 26, 2020 at 09:59:45PM +0530, Jerin Jacob wrote:
> > > > > > > > On Tue, May 26, 2020 at 9:36 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > > > > > > > On Tue, May 26, 2020 at 01:09:37PM +0530, Jerin Jacob wrote:
> > > > > > > > > > On Tue, May 26, 2020 at 2:54 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Since dynamic fields and flags were added in 19.11,
> > > > > > > > > > > the idea was to use them for new features, not only PMD-specific.
> > > > > > > > > > >
> > > > > > > > > > > The rule is made more explicit in doxygen, in the mbuf guide,
> > > > > > > > > > > and in the contribution design guidelines.
> > > > > > > > > > >
> > > > > > > > > > > For more information about the original design, see the presentation
> > > > > > > > > > > https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > > > > ---
> > > > > > > > > > >  doc/guides/contributing/design.rst | 13 +++++++++++++
> > > > > > > > > > >  doc/guides/prog_guide/mbuf_lib.rst | 23 +++++++++++++++++++++++
> > > > > > > > > > >  lib/librte_mbuf/rte_mbuf_core.h    |  2 ++
> > > > > > > > > > >  3 files changed, 38 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/doc/guides/contributing/design.rst b/doc/guides/contributing/design.rst
> > > > > > > > > > > index d3dd694b65..508115d5bd 100644
> > > > > > > > > > > --- a/doc/guides/contributing/design.rst
> > > > > > > > > > > +++ b/doc/guides/contributing/design.rst
> > > > > > > > > > > +Mbuf features
> > > > > > > > > > > +-------------
> > > > > > > > > > > +
> > > > > > > > > > > +The ``rte_mbuf`` structure must be kept small (128 bytes).
> > > > > > > > > > > +
> > > > > > > > > > > +In order to add new features without wasting buffer space for unused features,
> > > > > > > > > > > +some fields and flags can be registered dynamically in a shared area.
> > > > > > > > > >
> > > > > > > > > > I think, instead of "can", it should be "must"
> > > > > > > > > >
> > > > > > > > > > > +The "dynamic" mbuf area is the default choice for the new features.
> > > > > > > > >
> > > > > > > > > In my opinion, Thomas' proposal is correct, with the next sentence
> > > > > > > > > saying it is the default choice for new features.
> > > > > > > > >
> > > > > > > > > Giving guidelines is a good thing (thanks Thomas for documenting it),
> > > > > > > > > but I don't think we should be too strict: the door remains open for
> > > > > > > > > technical debate and exceptions.
> > > > > > > >
> > > > > > > > If you are open for the exception then it must be mention in what case
> > > > > > > > the exception is allowed and what are the criteria of the exception?
> > > > > > > >
> > > > > > > > For example,  Why did n't we choose the following patch as expectation
> > > > > > > > http://patches.dpdk.org/patch/68733/  even if only one bit used.
> > > > > > > >
> > > > > > > > If we are not not defining the criteria, IMO, This patch serve no purpose than
> > > > > > > > the existing situation.
> > > > > > > >
> > > > > > > > Do you think, any case where the dynamic scheme can not be used as a replacement
> > > > > > > > for static other than performance hit.
> > > > > > >
> > > > > > > I don't think it is possible to anticipate all criteria in the
> > > > > > > documentation. With Thomas' proposal, it gives a direction is and a
> > > > > > > global view, but it must not completly replace reflection and
> > > > > > > discussion.
> > > > > >
> > > > > > I don't think, we need to anticipate all the criteria in the documentation.
> > > > > > At least ONE should be given as an example of an exception.
> > > > >
> > > > > I think it is too early to be more specific in the guidelines.
> > > > > Do we agree this patch is a first good step in the documentation?
> > > >
> > > > IMO, there is a gap. The subject says the rule, but no rule here.
> > > > We are just giving some guideline and following info in the patch
> > > > given by Olivier is not
> > > > expressed if we read the patch.
> > > >
> > > > "
> > > >  I don't think we should be too strict: the door remains open for
> > > > technical debate and exceptions.
> > > > "
> > >
> > > Indeed, the headline should be
> > >         mbuf: add guideline for new fields and flags
> > >
> > >
> > > > > We can extend the guidelines a bit later after having some
> > > > > discussions on specific cases, and probably in the techboard too.
> > > > >
> > > > >
> > > > > > I would say,
> > > > > > a) If a feature takes only one bit and its part of normative API spec
> > > > > > and it used in fastpath we should consider the static scheme.
> > > > > > b) Adding an exception to the existing list needs approval at least
> > > > > > from three maintainers
> > > > > >
> > > > > > For me, it is a very legitimate case to have support for
> > > > > > http://patches.dpdk.org/patch/68733/ to the static scheme
> > > > > > as it takes 1 bit for a feature and it is part of the normative spec.
> > > > > > I don't get in explanation in the ml, why
> > > > > > we can not make it as the static scheme for this case.
> > > > >
> > > > > We can continue discussion about this specific case in the right thread.
> > > >
> > > > Yes. The email thread[1] provided all the details. We have optimized
> > > > to one bit for this feature.
> > > > We are expecting Olivier to comment on the new proposal.
> > > > [1]
> > > > http://patches.dpdk.org/patch/68733/
> > > >
> > > > > Note: I don't have a definitive opinion on it, I need to read it carefully.
> > > >
> > > > Please read it carefully and please provide any technical opinions if
> > > > you have any.
> > > >
> > > >
> > > > > > My worry is, if we are keeping as open-ended means, we are giving room
> > > > > > for the disparity among the vendors/feature
> > > > > > as I dont think, There is use case where dynamic scheme can not be
> > > > > > used as a replacement
> > > > > > for static other than performance hit.(Could think of any use case?)
> > > > > > So open ended boils down to preference to specific feature/vendor. I
> > > > > > think,that path should be avoided.
> > > > >
> > > > > Of course all rules and decisions have to be fair.
> > > > > It's not even a question.
> > > >
> > > > Yes. But I dont think, this patch is not  enforcing anything such,
> > > > instead it makes it as an open-ended
> > > > for more confusion. IMO, if it not black and white then better to not
> > > > express the rule.
> > >
> > > I disagree about "more confusion".
> >
> > My confusion will clear up if
> > 1) s/rule/guide line/ change across the patch
>
> OK for me, guideline is indeed a better name.
>
> > 2) Explicit mention of the following or similar sentence.
> > it is a guideline and exception is allowed on a case by case using
> > technical debate.
>
> In my opinion, anything written in the documentation is questionable and
> discussable (because the code or the environment can change, or because someone
> can come with new arguments), and I don't really see the added value to
> explicitly say it... or we'll have to say it for every guideline in the
> documentation.

Is n't source of confusion? in the email we are saying exception is allowed.
But we dont want to document it? What is the harm in documenting IF IT
IS really allowed it? If it not documented, how someone know what is
agreed upon?
If the code or environment changes, we can change the patch as well based
on everyone's agreement based on the patch review.
IMO, What is agreed upon should be documented.

>
>
> >
> > > I think the value of this patch is to improve awareness
> > > about the need for using dynamic fields and flags.
> > >
> > > Let's ask other opinions about the added value of this patch.
> > >
> > >