DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob <jerinjacobk@gmail.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: Jerin Jacob <jerinj@marvell.com>,
	Olivier Matz <olivier.matz@6wind.com>, dpdk-dev <dev@dpdk.org>,
	David Marchand <david.marchand@redhat.com>,
	Nithin Dabilpuram <ndabilpuram@marvell.com>,
	 Krzysztof Kanas <kkanas@marvell.com>,
	Andrew Rybchenko <arybchenko@solarflare.com>,
	 Ferruh Yigit <ferruh.yigit@intel.com>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	 John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>
Subject: Re: [dpdk-dev] [PATCH] mbuf: document rule for new fields and flags
Date: Wed, 27 May 2020 17:37:13 +0530
Message-ID: <CALBAE1N4YvrTzqqNOzk2w0dJxMD3XEserjEP_ocb4_0fZoWPew@mail.gmail.com> (raw)
In-Reply-To: <3071819.TdKep6GQZa@thomas>

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.
>
>

  reply	other threads:[~2020-05-27 12:07 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-25 21:24 Thomas Monjalon
2020-05-26  7:39 ` Jerin Jacob
2020-05-26 16:06   ` Olivier Matz
2020-05-26 16:29     ` Jerin Jacob
2020-05-27  7:09       ` Olivier Matz
2020-05-27  7:31         ` Jerin Jacob
2020-05-27  9:51           ` Thomas Monjalon
2020-05-27 11:43             ` Jerin Jacob
2020-05-27 11:56               ` Thomas Monjalon
2020-05-27 12:07                 ` Jerin Jacob [this message]
2020-05-27 12:23                   ` Olivier Matz
2020-05-27 12:34                     ` Jerin Jacob
2020-06-11  6:32 ` [dpdk-dev] [PATCH v2] mbuf: document guideline " Thomas Monjalon
2020-06-11  6:37   ` Jerin Jacob
2020-06-11  7:30     ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALBAE1N4YvrTzqqNOzk2w0dJxMD3XEserjEP_ocb4_0fZoWPew@mail.gmail.com \
    --to=jerinjacobk@gmail.com \
    --cc=arybchenko@solarflare.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinj@marvell.com \
    --cc=john.mcnamara@intel.com \
    --cc=kkanas@marvell.com \
    --cc=marko.kovacevic@intel.com \
    --cc=ndabilpuram@marvell.com \
    --cc=olivier.matz@6wind.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git