From: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
To: Luca Vizzarro <Luca.Vizzarro@arm.com>
Cc: dev@dpdk.org, Paul Szczepanek <Paul.Szczepanek@arm.com>,
Thomas Monjalon <thomas@monjalon.net>,
Lijuan Tu <lijuan.tu@intel.com>
Subject: Re: [PATCH] dts: improve documentation
Date: Mon, 15 Jan 2024 10:36:08 +0100 [thread overview]
Message-ID: <CAOb5WZbtXrsri8v+14y+nEEH_Wa7bSyFc=bpwb3Qo28kMCkwMQ@mail.gmail.com> (raw)
In-Reply-To: <07ec0377-396a-46e9-93e4-c7e5133441c5@arm.com>
On Fri, Jan 12, 2024 at 6:16 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Hi Juraj,
>
> Thank you for your review!
>
> On 12/01/2024 13:24, Juraj Linkeš wrote:
> > I have two extra suggestions apart from the comments below:
> > There's a typo inside the "How To Write a Test Suite" section:
> > In that case, nothing will happen when they're is executed.
> >
> > And Mypy is missing from the list of linters in the "DTS Developer
> > Tools" section, could you please add it?
>
> Ack.
>
> > Just out of curiosity, is this generated from the schema? It's a
> > pretty neat format, but maintaining it could be a nightmare without a
> > script that would always produce the same format.
>
> I originally found only one tool that would generate rst. The generated
> output was quite messy though. Only after hacking its few configuration
> options and the schema, it produced something decent. But as it is only
> available on pip and it would have to become a DPDK docs requirement to
> generate them, it wouldn't be acceptable.
>
This wouldn't need to be a hard requirement. It could just be a tool
that submitters could use as a starting point (which they would
optionally install and use).
> Unless someone comes up with a good tool that could match our needs,
> unfortunately manual work is the only solution for the time being...and
> I won't deny it took me a bit of time to format it. The only major
> problem is all the extra whitespaces and the alignment of the columns
> needed to make sphinx happy. Once most of the work is done though – as
> it is in this case, changing it from there shouldn't be too bad.
>
Alright, I hope people won't be too frustrated with it. :-) But it's
likely the section is not going to be changed that much so it's not a
big deal.
> > The section names look to be taken from the schema and all of the
> > terminology is taken from the schema. Would it make sense to use YAML
> > terminology here, since people will try to use this information in
> > YAML files? Or maybe explain what we mean by definitions, properties,
> > objects, arrays or maybe some other things which YAML doesn't specify.
>
> I understand your point of view. I had a quick look at the YAML glossary
> and I think it may be a lot more confusing than using generic software
> engineering terminology. Also we may not want to be constrained to YAML.
>
> The YAML glossary refers to what we would call dictionary in Python and
> objects in JSON as mappings. And arrays and lists as sequences. Maybe
> changing array to list could be a good idea. Objects instead is
> something I don't personally like either. I took it from the JSON schema
> to rst generator tool, as I am not sure what fits best here.
>
I like mappings [0] and sequences [1], as that is the standard Python
terminology.
[0] https://docs.python.org/3/library/stdtypes.html#mapping-types-dict
[1] https://docs.python.org/3/library/stdtypes.html#sequence-types-list-tuple-range
> I welcome suggestions to change specific terms. On the other hand, I am
> not so sure about explaining them as it is out of scope. Moreover, the
> configuration template should cover all of the scenarios, so one can
> also infer usage from there.
>
One more thing to like about mappings and sequences, we don't (or
shouldn't) have to explain those. I think those would work fine on
both fronts.
> > We should update "amount" (uncountable) to something that's countable,
> > such as count or number.
>
> Ack.
>
> > Of note here is that some traffic generators (to which the port config
> > also applies) won't be using os_driver_for_dpdk (such as Scapy), but
> > rather os_driver, so the use is broader.
>
> Ack. Thanks for the explanation, makes sense.
>
> > The last newline is missing.
>
> Ack.
>
> > Let's keep the note that the skip_smoke_tests flag is optional
>
> Ack.
>
> > The traffic generator may need this core configuration. However, since
> > it's not required for all traffic generators (such as Scapy), we could
> > just move to the traffic_generator section. That would require some
> > code modifications though, but even the removal of lcores and
> > use_first_core should be addressed in the configuration classes as
> > well. Have you tried running DTS with these changes?
>
> Please correct me if I'm wrong. At the current state of DTS it seems
> that these two properties are only used with DPDK. If this is correct,
> it may be misleading to the user to add them for the traffic generator
> node, hinting that they may make a difference when they don't.
>
That's right, it's only used in DPDK.
> It would definitely makes sense to have dedicated properties for DPDK
> and for the traffic generators, instead of common ones.
The dedicated DPDK/TG properties look like the way to go, it just makes sense.
> But this is
> possibly more of a concern when and if support for other traffic
> generators will be added in the future.
>
We'll need to add support for at least one traffic generator that's
suitable for performance testing (Scapy is too slow) and then we'll
need to extra config (such as cores for T-Rex).
> The removal of `lcores` and `use_first_core` does not affect the
> execution as both have default values they fallback to, as seen in
> `dts/framework/config/__init__.py#NodeConfiguration.from_dict`.
>
> Yes, I tried running DTS and wasn't met by anything unusual.
Ah, the defaults made it work. Let's remove the tg node config from
the yaml file then, as you suggested.
>
> Best,
> Luca
next prev parent reply other threads:[~2024-01-15 9:36 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-03 12:54 Luca Vizzarro
2024-01-04 10:52 ` Thomas Monjalon
2024-01-04 12:34 ` Luca Vizzarro
2024-01-04 13:15 ` Thomas Monjalon
2024-01-12 13:24 ` Juraj Linkeš
2024-01-12 17:16 ` Luca Vizzarro
2024-01-15 9:36 ` Juraj Linkeš [this message]
2024-01-15 11:44 ` Luca Vizzarro
2024-01-16 11:44 ` [PATCH v2 1/2] " Luca Vizzarro
2024-01-16 11:44 ` [PATCH v2 2/2] dts: add configuration schema docs Luca Vizzarro
2024-01-16 16:48 ` Juraj Linkeš
2024-01-16 16:47 ` [PATCH v2 1/2] dts: improve documentation Juraj Linkeš
2024-01-19 17:29 ` 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='CAOb5WZbtXrsri8v+14y+nEEH_Wa7bSyFc=bpwb3Qo28kMCkwMQ@mail.gmail.com' \
--to=juraj.linkes@pantheon.tech \
--cc=Luca.Vizzarro@arm.com \
--cc=Paul.Szczepanek@arm.com \
--cc=dev@dpdk.org \
--cc=lijuan.tu@intel.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).