[05/12] gen: add raw packet data API and tests

Message ID 20211214141242.3383831-6-ronan.randles@intel.com (mailing list archive)
State Not Applicable, archived
Delegated to: Thomas Monjalon
Headers
Series add packet generator library and example app |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Ronan Randles Dec. 14, 2021, 2:12 p.m. UTC
  From: Harry van Haaren <harry.van.haaren@intel.com>

This commit adds a new API to gen, allowing the caller to set
raw packet data with a size. Tests are added to test the new
API with randomized packet data.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 app/test/test_gen.c | 33 ++++++++++++++++++++++++
 lib/gen/rte_gen.c   | 62 +++++++++++++++++++++++++++++++++++++++++----
 lib/gen/rte_gen.h   |  9 +++++++
 lib/gen/version.map |  1 +
 4 files changed, 100 insertions(+), 5 deletions(-)
  

Comments

Jerin Jacob Dec. 15, 2021, 12:40 p.m. UTC | #1
On Tue, Dec 14, 2021 at 7:43 PM Ronan Randles <ronan.randles@intel.com> wrote:
>
> From: Harry van Haaren <harry.van.haaren@intel.com>
>
> This commit adds a new API to gen, allowing the caller to set
> raw packet data with a size. Tests are added to test the new
> API with randomized packet data.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  }
>
> @@ -32,9 +53,37 @@ rte_gen_create(struct rte_mempool *mempool)
>  void
>  rte_gen_destroy(struct rte_gen *gen)
>  {
> +       rte_pktmbuf_free(gen->base_pkt);
>         rte_free(gen);
>  }
>

> +
>  uint16_t
>  rte_gen_rx_burst(struct rte_gen *gen,
>                  struct rte_mbuf **rx_pkts,
> @@ -45,17 +94,20 @@ rte_gen_rx_burst(struct rte_gen *gen,
>         if (err)
>                 return 0;
>
> -       const uint32_t pkt_len = 64;
> +       if (!gen->base_pkt)
> +               return 0;
> +
> +       const uint32_t base_size = gen->base_pkt->pkt_len;
> +       const uint8_t *base_data = rte_pktmbuf_mtod(gen->base_pkt, uint8_t *);

I think, the very next feature will be generating packets for
incrementing IP addresses or so. In this case, one packet-based
template will not work.
May we worth consider that use case into API framework first and add support
later for implementation as it may change the complete land space of API to have
better performance. Options like struct rte_gen logical object can have
N templates instead of one is an option on the table. :-)


>
>         uint32_t i;
>         for (i = 0; i < nb_pkts; i++) {
>                 struct rte_mbuf *m = rx_pkts[i];
>                 uint8_t *pkt_data = rte_pktmbuf_mtod(m, uint8_t *);
>
> -               memset(pkt_data, 0, pkt_len);
> -
> -               m->pkt_len  = pkt_len;
> -               m->data_len = pkt_len;
> +               rte_memcpy(pkt_data, base_data, base_size);
> +               m->pkt_len = base_size;
> +               m->data_len = base_size;
>         }
>
  
Van Haaren, Harry Dec. 17, 2021, 11:40 a.m. UTC | #2
+CC Thomas;

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, December 15, 2021 12:41 PM
> To: Randles, Ronan <ronan.randles@intel.com>
> Cc: dpdk-dev <dev@dpdk.org>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: Re: [PATCH 05/12] gen: add raw packet data API and tests
> 
> On Tue, Dec 14, 2021 at 7:43 PM Ronan Randles <ronan.randles@intel.com>
> wrote:
> >
> > From: Harry van Haaren <harry.van.haaren@intel.com>

<snip some patch contents>

> > +       const uint32_t base_size = gen->base_pkt->pkt_len;
> > +       const uint8_t *base_data = rte_pktmbuf_mtod(gen->base_pkt, uint8_t
> *);
> 
> I think, the very next feature will be generating packets for
> incrementing IP addresses or so.

Hah, yes! It’s a logical next step, and indeed we have POC code internally that Ronan
and I have worked on that does this :) I've been using this internal POC of
testing of OVS for ~ a year now, and it provides a pretty nice workflow for me.

> In this case, one packet-based template will not work.

Why not? I agree that "pre-calculating" all packets will not work, but the approach
we have taken for this library is different. See below;

> May we worth consider that use case into API framework first and add support
> later for implementation as it may change the complete land space of API to have
> better performance. Options like struct rte_gen logical object can have
> N templates instead of one is an option on the table. :-)

Agree - more complex usages have been designed for too. Let me explain;

1) A single gen instance uses a single template, and has "modifiers" that allow
manipulation of the packet before sending. The gen->base_pkt is copied to the
destination mbuf, and then updated by the modifiers. This approach is much better
to allow for huge flow-counts (> 1 million?) as pre-calculating and storing 1 million
packets is a waste of memory, and causes a lot of mem-IO for the datapath core.

2) The "modifiers" approach allows any number of things to be changed, with little
mem-IO, and variable CPU cycle cost based on the modifiers themselves.
If the CPU cycle cost of generating packets is too high, just add more cores :)

3) There are also some smarts we can apply for pre-calculating only a small amount of
data per packet (e.g. uniformly-random distributed src ip). The memory footprint is
lower than pre-calc of whole packets, and the runtime overhead of uniform-random
is moved to configure time instead of on the datapath.

4) Dynamically generating packets by modification of templates allows for cool things
to be added, e.g. adding timestamps to packets, and calculating latency can
be done using the modifier concept and a protocol string "Ether()/IP()/UDP()/TSC()".
If the packet is being decapped by the target application, the string params can provide
context for where to "retrieve" the TSC from on RX in the generator: "TSC(rx_offset=30)".
I've found this approach to be very flexible and nice, so am a big fan :)

5) In order to have multiple streams of totally-different traffic types (read "from multiple templates")
the user can initialize multiple rte_gen instances. This allows applications that require multi-stream traffic
to achieve that too, with the same abstraction as a single template stream. Initially the generator app is just
providing a single stream, but this application can be expanded to many usages over the next year before 22.11 :)

I could ramble on a bit more, but mostly diminishing returns I think... I'll just use this email as a reply to Thomas' tweet;
https://twitter.com/tmonjalo/status/1337313985662771201

Regards, -Harry
  
Thomas Monjalon Dec. 17, 2021, 4:19 p.m. UTC | #3
17/12/2021 12:40, Van Haaren, Harry:
> I could ramble on a bit more, but mostly diminishing returns I think...
> I'll just use this email as a reply to Thomas' tweet;
> https://twitter.com/tmonjalo/status/1337313985662771201

My original question was to know available applications,
not integrating such application in the DPDK repository.

I may me miss something obvious,
but I don't understand why trying to add a user app inside DPDK repo.
  
Van Haaren, Harry Dec. 20, 2021, 10:21 a.m. UTC | #4
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, December 17, 2021 4:19 PM
> To: Randles, Ronan <ronan.randles@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; dev@dpdk.org; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [PATCH 05/12] gen: add raw packet data API and tests
> 
> 17/12/2021 12:40, Van Haaren, Harry:
> > I could ramble on a bit more, but mostly diminishing returns I think...
> > I'll just use this email as a reply to Thomas' tweet;
> > https://twitter.com/tmonjalo/status/1337313985662771201
> 
> My original question was to know available applications,
> not integrating such application in the DPDK repository.
> 
> I may me miss something obvious,
> but I don't understand why trying to add a user app inside DPDK repo.

There are likely a few points-of-view on this particular topic; and I'm glad
you mention it so we can discuss it clearly here.

There are two main parts to this patchset, the first is a packet generation library,
with an easy to use string-based syntax. The *library* is designed to be extended in
future to a range of "useful stuff" to do while generating packets. The packet generation
*application* should have minimal features, and focus on ease-of-use (as suggested below).

In order to test the DPDK code, we need a variety of unit tests, and a sample-application to show
users how to use the library (as well as docs etc). For me, the interesting part is that it is a small
step from a simple sample-app just for testing to a minimal tool for high-rate packet generation. 

I think many users of DPDK first install DPDK, then wish for a tool to generate high traffic rates
to test DPDK, and end up with a usability problem; DPDK does not include a usable packet generator.

To highlight this point; our own DPDK Docs simply ignore the requirement of packet-generation to
actually have packets processed by skeleton: http://doc.dpdk.org/guides/sample_app_ug/skeleton.html 
Our "quick start" on the website uses PCAP vdevs (avoiding the problem)  https://core.dpdk.org/doc/quick-start/
Even searching the entire docs for "generate packet" doesn't give any relevant/useful results:
http://doc.dpdk.org/guides/search.html?q=generate+packet&check_keywords=yes&area=default# 

Users could internet-search & find pktgen, moongen, trex, or similar tools. These tools are fantastic for experienced
developers such as devs on this mailing list - we should *NOT* replicate these complex tools in DPDK itself. However,
building any tool outside of DPDK repo requires more effort; another git-clone, another set of dependencies to install,
perhaps another build-system to get used to. Particularly for people starting out with DPDK (who are likely finding
it difficult to learn the various hugepage/PCI-binding etc), this is yet another problem to solve, or to give up.

So my proposal is as follows; let us add a simple DPDK traffic generator to DPDK. We can define its scope
and its intended use, limiting the scope and capabilities. As before, I do NOT think it a good idea to build a
complex and feature-rich packet generator. I do feel it useful to have an easy-to-use application in DPDK that
is particularly designed for generating specific packets, at specific line-rates, and reports mpps returned.

Thoughts on adding an small scope-limited application to DPDK enabling ease-of-packet-generation for new users?

Regards, -Harry
  
Jerin Jacob Dec. 20, 2021, 1:21 p.m. UTC | #5
On Fri, Dec 17, 2021 at 5:10 PM Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
>
> +CC Thomas;
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Wednesday, December 15, 2021 12:41 PM
> > To: Randles, Ronan <ronan.randles@intel.com>
> > Cc: dpdk-dev <dev@dpdk.org>; Van Haaren, Harry
> > <harry.van.haaren@intel.com>
> > Subject: Re: [PATCH 05/12] gen: add raw packet data API and tests
> >
> > On Tue, Dec 14, 2021 at 7:43 PM Ronan Randles <ronan.randles@intel.com>
> > wrote:
> > >
> > > From: Harry van Haaren <harry.van.haaren@intel.com>
>
> <snip some patch contents>
>
> > > +       const uint32_t base_size = gen->base_pkt->pkt_len;
> > > +       const uint8_t *base_data = rte_pktmbuf_mtod(gen->base_pkt, uint8_t
> > *);
> >
> > I think, the very next feature will be generating packets for
> > incrementing IP addresses or so.
>
> Hah, yes! It’s a logical next step, and indeed we have POC code internally that Ronan
> and I have worked on that does this :) I've been using this internal POC of
> testing of OVS for ~ a year now, and it provides a pretty nice workflow for me.
>
> > In this case, one packet-based template will not work.
>
> Why not? I agree that "pre-calculating" all packets will not work, but the approach
> we have taken for this library is different. See below;
>
> > May we worth consider that use case into API framework first and add support
> > later for implementation as it may change the complete land space of API to have
> > better performance. Options like struct rte_gen logical object can have
> > N templates instead of one is an option on the table. :-)
>
> Agree - more complex usages have been designed for too. Let me explain;
>
> 1) A single gen instance uses a single template, and has "modifiers" that allow
> manipulation of the packet before sending. The gen->base_pkt is copied to the
> destination mbuf, and then updated by the modifiers. This approach is much better
> to allow for huge flow-counts (> 1 million?) as pre-calculating and storing 1 million
> packets is a waste of memory, and causes a lot of mem-IO for the datapath core.
>
> 2) The "modifiers" approach allows any number of things to be changed, with little
> mem-IO, and variable CPU cycle cost based on the modifiers themselves.
> If the CPU cycle cost of generating packets is too high, just add more cores :)
>
> 3) There are also some smarts we can apply for pre-calculating only a small amount of
> data per packet (e.g. uniformly-random distributed src ip). The memory footprint is
> lower than pre-calc of whole packets, and the runtime overhead of uniform-random
> is moved to configure time instead of on the datapath.
>
> 4) Dynamically generating packets by modification of templates allows for cool things
> to be added, e.g. adding timestamps to packets, and calculating latency can
> be done using the modifier concept and a protocol string "Ether()/IP()/UDP()/TSC()".
> If the packet is being decapped by the target application, the string params can provide
> context for where to "retrieve" the TSC from on RX in the generator: "TSC(rx_offset=30)".
> I've found this approach to be very flexible and nice, so am a big fan :)
>
> 5) In order to have multiple streams of totally-different traffic types (read "from multiple templates")
> the user can initialize multiple rte_gen instances. This allows applications that require multi-stream traffic
> to achieve that too, with the same abstraction as a single template stream. Initially the generator app is just
> providing a single stream, but this application can be expanded to many usages over the next year before 22.11 :)

OK. I thought "modifiers" will need some sort of critical section in
multiple producer use cases. If so,
one option could be N streams in one gen instance vs N gen instance.
Just my 2c. Anyway, you folks can decide
on one option vs another. Only my concern was including such features
affect the prototype of existing APIs or not?
In either case, No strong opinion.


>
> I could ramble on a bit more, but mostly diminishing returns I think... I'll just use this email as a reply to Thomas' tweet;
> https://twitter.com/tmonjalo/status/1337313985662771201
>
> Regards, -Harry
  
Thomas Monjalon Jan. 19, 2022, 2:56 p.m. UTC | #6
20/12/2021 11:21, Van Haaren, Harry:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 17/12/2021 12:40, Van Haaren, Harry:
> > > I could ramble on a bit more, but mostly diminishing returns I think...
> > > I'll just use this email as a reply to Thomas' tweet;
> > > https://twitter.com/tmonjalo/status/1337313985662771201
> > 
> > My original question was to know available applications,
> > not integrating such application in the DPDK repository.
> > 
> > I may me miss something obvious,
> > but I don't understand why trying to add a user app inside DPDK repo.
> 
> There are likely a few points-of-view on this particular topic; and I'm glad
> you mention it so we can discuss it clearly here.
> 
> There are two main parts to this patchset, the first is a packet generation library,
> with an easy to use string-based syntax. The *library* is designed to be extended in
> future to a range of "useful stuff" to do while generating packets.

The text syntax would be specific to this application
and not usable somewhere else, so it doesn't make sense as a lib.

> The packet generation
> *application* should have minimal features, and focus on ease-of-use (as suggested below).

It would be either a limited application,
or an ever-growing application.
If the latter, it should not be in the main DPDK repository in my opinion.

By the way, I don't think it is the responsibility of DPDK to generate packets.
I would prefer having an application using the already known scapy
or a graphical interface like Ostinato.
There are tons of approach to define packets to send (pCraft is another one).
DPDK should only manage the Tx part, and optionally Rx of forwarded packets.

> In order to test the DPDK code, we need a variety of unit tests, and a sample-application to show
> users how to use the library (as well as docs etc). For me, the interesting part is that it is a small
> step from a simple sample-app just for testing to a minimal tool for high-rate packet generation.
> 
> I think many users of DPDK first install DPDK, then wish for a tool to generate high traffic rates
> to test DPDK, and end up with a usability problem; DPDK does not include a usable packet generator.

I don't see any usability problem in using an external well known tool.
Learning a new tool provided by DPDK *is* a usabilty difficulty.

> To highlight this point; our own DPDK Docs simply ignore the requirement of packet-generation to
> actually have packets processed by skeleton: http://doc.dpdk.org/guides/sample_app_ug/skeleton.html 
> Our "quick start" on the website uses PCAP vdevs (avoiding the problem)  https://core.dpdk.org/doc/quick-start/
> Even searching the entire docs for "generate packet" doesn't give any relevant/useful results:
> http://doc.dpdk.org/guides/search.html?q=generate+packet&check_keywords=yes&area=default# 
> 
> Users could internet-search & find pktgen, moongen, trex, or similar tools. These tools are fantastic for experienced
> developers such as devs on this mailing list - we should *NOT* replicate these complex tools in DPDK itself. However,
> building any tool outside of DPDK repo requires more effort; another git-clone, another set of dependencies to install,
> perhaps another build-system to get used to. Particularly for people starting out with DPDK (who are likely finding
> it difficult to learn the various hugepage/PCI-binding etc), this is yet another problem to solve, or to give up.
> 
> So my proposal is as follows; let us add a simple DPDK traffic generator to DPDK. We can define its scope
> and its intended use, limiting the scope and capabilities. As before, I do NOT think it a good idea to build a
> complex and feature-rich packet generator. I do feel it useful to have an easy-to-use application in DPDK that
> is particularly designed for generating specific packets, at specific line-rates, and reports mpps returned.
> 
> Thoughts on adding an small scope-limited application to DPDK enabling ease-of-packet-generation for new users?

So you want a simple packet generator for simple benchmarks?
And for complex benchmarks, we use another tool?
  
Van Haaren, Harry Jan. 20, 2022, 10:21 a.m. UTC | #7
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, January 19, 2022 2:56 PM
> To: Randles, Ronan <ronan.randles@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org; Jerin Jacob <jerinjacobk@gmail.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [PATCH 05/12] gen: add raw packet data API and tests
> 
> 20/12/2021 11:21, Van Haaren, Harry:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 17/12/2021 12:40, Van Haaren, Harry:
> > > > I could ramble on a bit more, but mostly diminishing returns I think...
> > > > I'll just use this email as a reply to Thomas' tweet;
> > > > https://twitter.com/tmonjalo/status/1337313985662771201
> > >
> > > My original question was to know available applications,
> > > not integrating such application in the DPDK repository.
> > >
> > > I may me miss something obvious,
> > > but I don't understand why trying to add a user app inside DPDK repo.
> >
> > There are likely a few points-of-view on this particular topic; and I'm glad
> > you mention it so we can discuss it clearly here.
> >
> > There are two main parts to this patchset, the first is a packet generation
> library,
> > with an easy to use string-based syntax. The *library* is designed to be
> extended in
> > future to a range of "useful stuff" to do while generating packets.
> 
> The text syntax would be specific to this application
> and not usable somewhere else, so it doesn't make sense as a lib.

The text string format "Ether()/IP(src=1.2.3.4)/UDP()" is not application specific,
and the items being parsed can be easily added to; that's what I meant above.


> > The packet generation
> > *application* should have minimal features, and focus on ease-of-use (as
> suggested below).
> 
> It would be either a limited application,
> or an ever-growing application.
> If the latter, it should not be in the main DPDK repository in my opinion.
> 
> By the way, I don't think it is the responsibility of DPDK to generate packets.
> I would prefer having an application using the already known scapy
> or a graphical interface like Ostinato.
> There are tons of approach to define packets to send (pCraft is another one).
> DPDK should only manage the Tx part, and optionally Rx of forwarded packets.

Scapy did not meet the performance requirements for infrastructure testing use-cases.
The goal of gen lib is to provide a toolbox for high-speed "scapy-like" packet generation,
the above tools didn't seem like the right fit for my needs; hence Gen library.

> > In order to test the DPDK code, we need a variety of unit tests, and a sample-
> application to show
> > users how to use the library (as well as docs etc). For me, the interesting part is
> that it is a small
> > step from a simple sample-app just for testing to a minimal tool for high-rate
> packet generation.
> >
> > I think many users of DPDK first install DPDK, then wish for a tool to generate
> high traffic rates
> > to test DPDK, and end up with a usability problem; DPDK does not include a
> usable packet generator.
> 
> I don't see any usability problem in using an external well known tool.
> Learning a new tool provided by DPDK *is* a usabilty difficulty.
> 
> > To highlight this point; our own DPDK Docs simply ignore the requirement of
> packet-generation to
> > actually have packets processed by skeleton:
> http://doc.dpdk.org/guides/sample_app_ug/skeleton.html
> > Our "quick start" on the website uses PCAP vdevs (avoiding the problem)
> https://core.dpdk.org/doc/quick-start/
> > Even searching the entire docs for "generate packet" doesn't give any
> relevant/useful results:
> >
> http://doc.dpdk.org/guides/search.html?q=generate+packet&check_keywords=
> yes&area=default#
> >
> > Users could internet-search & find pktgen, moongen, trex, or similar tools.
> These tools are fantastic for experienced
> > developers such as devs on this mailing list - we should *NOT* replicate these
> complex tools in DPDK itself. However,
> > building any tool outside of DPDK repo requires more effort; another git-clone,
> another set of dependencies to install,
> > perhaps another build-system to get used to. Particularly for people starting
> out with DPDK (who are likely finding
> > it difficult to learn the various hugepage/PCI-binding etc), this is yet another
> problem to solve, or to give up.
> >
> > So my proposal is as follows; let us add a simple DPDK traffic generator to DPDK.
> We can define its scope
> > and its intended use, limiting the scope and capabilities. As before, I do NOT
> think it a good idea to build a
> > complex and feature-rich packet generator. I do feel it useful to have an easy-
> to-use application in DPDK that
> > is particularly designed for generating specific packets, at specific line-rates, and
> reports mpps returned.
> >
> > Thoughts on adding an small scope-limited application to DPDK enabling ease-
> of-packet-generation for new users?
> 
> So you want a simple packet generator for simple benchmarks?
> And for complex benchmarks, we use another tool?

In short, yes. Make simple things easy (gen lib in DPDK itself), and make difficult things
possible (by using TRex, Ostinato, Warp17, MoonGen, Pktgen, pCraft, Scapy, or other tools).

But to summarize, it seems there are multiple questions/concerns around merging the
gen library into DPDK. That's OK, this is not a critical feature to upstream, I thought it
might be useful to the wider community.

As some rework was done from the V1 to a V2, adding some protocols and generally
advancing the library, Ronan and I will post the V2 to list to make it publicly available.
We can mark the V2 as "not applicable" in patchwork once sent.

I will likely continue to use the Gen library for my own packet-generation & testing
needs. If in future somebody is interested in Gen library send me an email!

Regards, -Harry
  
Van Haaren, Harry Jan. 21, 2022, 10:45 a.m. UTC | #8
> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Thursday, January 20, 2022 10:21 AM
> To: 'Thomas Monjalon' <thomas@monjalon.net>; Randles, Ronan
> <ronan.randles@intel.com>
> Cc: dev@dpdk.org; Jerin Jacob <jerinjacobk@gmail.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: RE: [PATCH 05/12] gen: add raw packet data API and tests

<snip previous discussion>

> But to summarize, it seems there are multiple questions/concerns around
> merging the gen library into DPDK. That's OK, this is not a critical feature to
> upstream, I thought it might be useful to the wider community.
> 
> As some rework was done from the V1 to a V2, adding some protocols and
> generally
> advancing the library, Ronan and I will post the V2 to list to make it publicly
> available.
> We can mark the V2 as "not applicable" in patchwork once sent.

As promised, the reworked V2 patches are available here;
http://patches.dpdk.org/project/dpdk/cover/20220121103122.2926856-1-ronan.randles@intel.com/

They are marked as "not applicable" in patchwork, so will not be candidates
for merge, but are available for interested folks to work with.

I'd like to thank Ronan for his work on the Gen library during his internship,
it is useful tool for my testing usages, and I expect to continue using it in future!

Thanks community for reviews and suggestions on improving post-v1.

Regards, -Harry

<snip rest of email>
  
Xueming Li Jan. 21, 2022, 2:20 p.m. UTC | #9
On Mon, 2021-12-20 at 18:51 +0530, Jerin Jacob wrote:
> On Fri, Dec 17, 2021 at 5:10 PM Van Haaren, Harry
> <harry.van.haaren@intel.com> wrote:
> > 
> > +CC Thomas;
> > 
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Wednesday, December 15, 2021 12:41 PM
> > > To: Randles, Ronan <ronan.randles@intel.com>
> > > Cc: dpdk-dev <dev@dpdk.org>; Van Haaren, Harry
> > > <harry.van.haaren@intel.com>
> > > Subject: Re: [PATCH 05/12] gen: add raw packet data API and tests
> > > 
> > > On Tue, Dec 14, 2021 at 7:43 PM Ronan Randles <ronan.randles@intel.com>
> > > wrote:
> > > > 
> > > > From: Harry van Haaren <harry.van.haaren@intel.com>
> > 
> > <snip some patch contents>
> > 
> > > > +       const uint32_t base_size = gen->base_pkt->pkt_len;
> > > > +       const uint8_t *base_data = rte_pktmbuf_mtod(gen->base_pkt, uint8_t
> > > *);
> > > 
> > > I think, the very next feature will be generating packets for
> > > incrementing IP addresses or so.
> > 
> > Hah, yes! It’s a logical next step, and indeed we have POC code internally that Ronan
> > and I have worked on that does this :) I've been using this internal POC of
> > testing of OVS for ~ a year now, and it provides a pretty nice workflow for me.
> > 
> > > In this case, one packet-based template will not work.
> > 
> > Why not? I agree that "pre-calculating" all packets will not work, but the approach
> > we have taken for this library is different. See below;
> > 
> > > May we worth consider that use case into API framework first and add support
> > > later for implementation as it may change the complete land space of API to have
> > > better performance. Options like struct rte_gen logical object can have
> > > N templates instead of one is an option on the table. :-)
> > 
> > Agree - more complex usages have been designed for too. Let me explain;
> > 
> > 1) A single gen instance uses a single template, and has "modifiers" that allow
> > manipulation of the packet before sending. The gen->base_pkt is copied to the
> > destination mbuf, and then updated by the modifiers. This approach is much better
> > to allow for huge flow-counts (> 1 million?) as pre-calculating and storing 1 million
> > packets is a waste of memory, and causes a lot of mem-IO for the datapath core.
> > 
> > 2) The "modifiers" approach allows any number of things to be changed, with little
> > mem-IO, and variable CPU cycle cost based on the modifiers themselves.
> > If the CPU cycle cost of generating packets is too high, just add more cores :)
> > 
> > 3) There are also some smarts we can apply for pre-calculating only a small amount of
> > data per packet (e.g. uniformly-random distributed src ip). The memory footprint is
> > lower than pre-calc of whole packets, and the runtime overhead of uniform-random
> > is moved to configure time instead of on the datapath.
> > 
> > 4) Dynamically generating packets by modification of templates allows for cool things
> > to be added, e.g. adding timestamps to packets, and calculating latency can
> > be done using the modifier concept and a protocol string "Ether()/IP()/UDP()/TSC()".
> > If the packet is being decapped by the target application, the string params can provide
> > context for where to "retrieve" the TSC from on RX in the generator: "TSC(rx_offset=30)".
> > I've found this approach to be very flexible and nice, so am a big fan :)
> > 
> > 5) In order to have multiple streams of totally-different traffic types (read "from multiple templates")
> > the user can initialize multiple rte_gen instances. This allows applications that require multi-stream traffic
> > to achieve that too, with the same abstraction as a single template stream. Initially the generator app is just
> > providing a single stream, but this application can be expanded to many usages over the next year before 22.11 :)
> 
> OK. I thought "modifiers" will need some sort of critical section in
> multiple producer use cases. If so,
> one option could be N streams in one gen instance vs N gen instance.
> Just my 2c. Anyway, you folks can decide
> on one option vs another. Only my concern was including such features
> affect the prototype of existing APIs or not?
> In either case, No strong opinion.

Sometimes we need RSS with limited streams, modifies need to copy
template data, then modify accordingly, involves more cycles and data
cache, not a good choice for performance test.

By cloning a list of templates, just copy the mbuf headers, seems more
efficient for such case.

Agree modifiers a flexible way to do things more powerful, hopefully we
have them all :)

> 
> 
> > 
> > I could ramble on a bit more, but mostly diminishing returns I think... I'll just use this email as a reply to Thomas' tweet;
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Ftmonjalo%2Fstatus%2F1337313985662771201&amp;data=04%7C01%7Cxuemingl%40nvidia.com%7Cedc55ae350504eaebe0b08d9c3bbb464%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637756033240578829%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=JVkpoweUrPoEWf7rWg1tSG4qiO9IKTtnw30x%2BqBZb%2FI%3D&amp;reserved=0
> > 
> > Regards, -Harry
  

Patch

diff --git a/app/test/test_gen.c b/app/test/test_gen.c
index ceacd8c7c4..b60ceaef8a 100644
--- a/app/test/test_gen.c
+++ b/app/test/test_gen.c
@@ -5,6 +5,7 @@ 
 #include <rte_common.h>
 #include <rte_gen.h>
 #include <rte_mbuf.h>
+#include <rte_random.h>
 
 #include "test.h"
 
@@ -75,6 +76,37 @@  test_gen_loop_rxtx(void)
 
 		total_sent += nb_tx;
 	}
+	rte_gen_destroy(gen);
+	return 0;
+}
+
+static int
+test_gen_packet_set_raw(void)
+{
+	struct rte_gen *gen = rte_gen_create(mp);
+	TEST_ASSERT_FAIL(gen, "Expected valid pointer after create()");
+
+	/* Set a raw packet payload, and ensure the next received packet has
+	 * that packet data as contents and size.
+	 */
+	uint64_t pkt_data[8];
+	uint32_t i;
+	for (i = 0; i < 8; i++)
+		pkt_data[i] = rte_rand();
+
+	int32_t err = rte_gen_packet_set_raw(gen, (void *)pkt_data, 64);
+	TEST_ASSERT_EQUAL(err, 0, "Expected set raw() to return success.");
+
+	struct rte_mbuf *bufs[BURST_MAX];
+	uint16_t nb_rx = rte_gen_rx_burst(gen, bufs, 1);
+	TEST_ASSERT_EQUAL(nb_rx, 1, "Expected rx packet burst.");
+
+	void *mbuf_data = rte_pktmbuf_mtod(bufs[0], void *);
+	int32_t data_equal = memcmp(pkt_data, mbuf_data, 64) == 0;
+	TEST_ASSERT_EQUAL(data_equal, 1,
+		"Expected packet data equal to input data.");
+
+	rte_pktmbuf_free(bufs[0]);
 
 	rte_gen_destroy(gen);
 	return 0;
@@ -88,6 +120,7 @@  static struct unit_test_suite gen_suite  = {
 		TEST_CASE_ST(NULL, NULL, test_gen_create),
 		TEST_CASE_ST(NULL, NULL, test_gen_basic_rxtx),
 		TEST_CASE_ST(NULL, NULL, test_gen_loop_rxtx),
+		TEST_CASE_ST(NULL, NULL, test_gen_packet_set_raw),
 		TEST_CASES_END() /**< NULL terminate unit test array */
 	}
 };
diff --git a/lib/gen/rte_gen.c b/lib/gen/rte_gen.c
index f0ad57fa81..432be65f1a 100644
--- a/lib/gen/rte_gen.c
+++ b/lib/gen/rte_gen.c
@@ -6,13 +6,25 @@ 
 
 #include <rte_mbuf.h>
 #include <rte_malloc.h>
+#include <rte_hexdump.h>
+#include <rte_log.h>
+
+RTE_LOG_REGISTER(gen_logtype, lib.gen, NOTICE);
+
+#define TGEN_LOG(level, fmt, args...)				\
+	rte_log(RTE_LOG_ ## level, gen_logtype, "%s(): " fmt,	\
+		__func__, ## args)
 
 #define GEN_MAX_BURST 32
+#define GEN_INIT_PKT_SIZE 64
 
 /** Structure that represents a traffic generator. */
 struct rte_gen {
 	/* Mempool that buffers are retrieved from. */
 	struct rte_mempool *mp;
+
+	/* Packet template to send. */
+	struct rte_mbuf *base_pkt;
 };
 
 /* Allocate and initialize a traffic generator instance. */
@@ -25,6 +37,15 @@  rte_gen_create(struct rte_mempool *mempool)
 
 	gen->mp = mempool;
 
+	uint8_t data[GEN_INIT_PKT_SIZE];
+	memset(data, 0, GEN_INIT_PKT_SIZE);
+	int32_t err = rte_gen_packet_set_raw(gen, data, GEN_INIT_PKT_SIZE);
+	if (err) {
+		TGEN_LOG(ERR, "Failed to set initial packet\n");
+		rte_free(gen);
+		return NULL;
+	}
+
 	return gen;
 }
 
@@ -32,9 +53,37 @@  rte_gen_create(struct rte_mempool *mempool)
 void
 rte_gen_destroy(struct rte_gen *gen)
 {
+	rte_pktmbuf_free(gen->base_pkt);
 	rte_free(gen);
 }
 
+int32_t
+rte_gen_packet_set_raw(struct rte_gen *gen,
+		       const uint8_t *raw_data,
+		       uint32_t raw_data_size)
+{
+
+	struct rte_mbuf *new_pkt = rte_pktmbuf_alloc(gen->mp);
+	if (!new_pkt) {
+		TGEN_LOG(ERR, "Failed to retireve mbuf for parser\n");
+		return -ENOMEM;
+	}
+
+	uint8_t *base_data = rte_pktmbuf_mtod(new_pkt, uint8_t *);
+	new_pkt->pkt_len = raw_data_size;
+	new_pkt->data_len = raw_data_size;
+	rte_memcpy(base_data, raw_data, raw_data_size);
+
+	/* If old packet exists, free it. */
+	struct rte_mbuf *old_pkt = gen->base_pkt;
+	gen->base_pkt = new_pkt;
+
+	if (old_pkt)
+		rte_pktmbuf_free(old_pkt);
+
+	return 0;
+}
+
 uint16_t
 rte_gen_rx_burst(struct rte_gen *gen,
 		 struct rte_mbuf **rx_pkts,
@@ -45,17 +94,20 @@  rte_gen_rx_burst(struct rte_gen *gen,
 	if (err)
 		return 0;
 
-	const uint32_t pkt_len = 64;
+	if (!gen->base_pkt)
+		return 0;
+
+	const uint32_t base_size = gen->base_pkt->pkt_len;
+	const uint8_t *base_data = rte_pktmbuf_mtod(gen->base_pkt, uint8_t *);
 
 	uint32_t i;
 	for (i = 0; i < nb_pkts; i++) {
 		struct rte_mbuf *m = rx_pkts[i];
 		uint8_t *pkt_data = rte_pktmbuf_mtod(m, uint8_t *);
 
-		memset(pkt_data, 0, pkt_len);
-
-		m->pkt_len  = pkt_len;
-		m->data_len = pkt_len;
+		rte_memcpy(pkt_data, base_data, base_size);
+		m->pkt_len = base_size;
+		m->data_len = base_size;
 	}
 
 	return nb_pkts;
diff --git a/lib/gen/rte_gen.h b/lib/gen/rte_gen.h
index 09ee1e8872..c8d85a5b72 100644
--- a/lib/gen/rte_gen.h
+++ b/lib/gen/rte_gen.h
@@ -85,6 +85,15 @@  rte_gen_tx_burst(struct rte_gen *gen,
 		 uint64_t *pkt_latencies,
 		 const uint16_t nb_pkts);
 
+/* Update the packet being sent to the provided raw data.
+ * @retval 0 Success.
+ * @retval -ENOMEM No memory available.
+ */
+int32_t
+__rte_experimental
+rte_gen_packet_set_raw(struct rte_gen *gen,
+		       const uint8_t *raw_data,
+		       uint32_t raw_data_size);
 
 #ifdef __cplusplus
 }
diff --git a/lib/gen/version.map b/lib/gen/version.map
index bdd25add6f..d75e0b4bac 100644
--- a/lib/gen/version.map
+++ b/lib/gen/version.map
@@ -5,4 +5,5 @@  EXPERIMENTAL {
 	rte_gen_destroy;
 	rte_gen_rx_burst;
 	rte_gen_tx_burst;
+	rte_gen_packet_set_raw;
 };