Re: [dpdk-dev] [PATCH] mbuf: document rule for new fields and flags

[Thomas Monjalon <thomas@monjalon.net>]

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.