DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jeremy Spewock <jspewock@iol.unh.edu>
To: Nicholas Pratte <npratte@iol.unh.edu>
Cc: thomas@monjalon.net, Luca.Vizzarro@arm.com,
	Honnappa.Nagarahalli@arm.com,  probb@iol.unh.edu,
	yoan.picchi@foss.arm.com, wathsala.vithanage@arm.com,
	 juraj.linkes@pantheon.tech, paul.szczepanek@arm.com,
	dev@dpdk.org
Subject: Re: [PATCH v3 1/4] dts: add send_packets to test suites and rework packet addressing
Date: Fri, 26 Jul 2024 15:13:20 -0400	[thread overview]
Message-ID: <CAAA20UTiiGcMwOsF+2V7Yam2zLz4zP1K8Wt=fK7xBkV7AzGN8g@mail.gmail.com> (raw)
In-Reply-To: <CAKXZ7egEOy4BVbPRoQvut-3PZD94pjaUqgv-3KUitco4W-8-tQ@mail.gmail.com>

Thanks for the comments, I just had one clarifying question about
them, but otherwise I will address them in the next version.

On Fri, Jul 26, 2024 at 3:00 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> I'll make sure to look over the other parts of this series and leave
> reviews at some point next week, but I prioritized this since I will
> be using this patch at some point in my GRE suites.
>
>
> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> > index 694b2eba65..0b678ed62d 100644
> > --- a/dts/framework/test_suite.py
> > +++ b/dts/framework/test_suite.py
> > @@ -199,7 +199,7 @@ def send_packet_and_capture(
> >          Returns:
> >              A list of received packets.
> >          """
> > -        packet = self._adjust_addresses(packet)
> > +        packet = self._adjust_addresses([packet])[0]
> >          return self.tg_node.send_packet_and_capture(
> >              packet,
> >              self._tg_port_egress,
> > @@ -208,6 +208,18 @@ def send_packet_and_capture(
> >              duration,
> >          )
> >
> > +    def send_packets(
> > +        self,
> > +        packets: list[Packet],
> > +    ) -> None:
> > +        """Send packets using the traffic generator and do not capture received traffic.
> > +
> > +        Args:
> > +            packets: Packets to send.
> > +        """
> > +        packets = self._adjust_addresses(packets)
> > +        self.tg_node.send_packets(packets, self._tg_port_egress)
> > +
> >      def get_expected_packet(self, packet: Packet) -> Packet:
> >          """Inject the proper L2/L3 addresses into `packet`.
> >
> > @@ -219,39 +231,59 @@ def get_expected_packet(self, packet: Packet) -> Packet:
> >          """
> >          return self._adjust_addresses(packet, expected=True)
> >
> > -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
> > +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
> >          """L2 and L3 address additions in both directions.
> >
> > +        Only missing addresses are added to packets, existing addressed will not be overridden.
>
> addressed should be addresses. Only saw this because of Chrome's
> built-in grammar correction.

Good catch.

>
> > +
> >          Assumptions:
> >              Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
> >
> >          Args:
> > -            packet: The packet to modify.
> > +            packets: The packets to modify.
> >              expected: If :data:`True`, the direction is SUT -> TG,
> >                  otherwise the direction is TG -> SUT.
> >          """
> > -        if expected:
> > -            # The packet enters the TG from SUT
> > -            # update l2 addresses
> > -            packet.src = self._sut_port_egress.mac_address
> > -            packet.dst = self._tg_port_ingress.mac_address
> > +        ret_packets = []
> > +        for packet in packets:
> > +            default_pkt_src = type(packet)().src
> > +            default_pkt_dst = type(packet)().dst
>
> This is really just a probing question for my sake, but what is the
> difference between the solution you have above type(packet)().src and
> Ether().src? Is there a preferred means of doing this?

There isn't really a functional difference at all under the assumption
that every packet we send will start with an Ethernet header. This
obviously isn't an unreasonable assumption to make, so maybe I was
reaching for flexibility that isn't really needed here by making it
work with any theoretical first layer that has a source address. I
wanted to do the same thing for the payload, but that causes issues
when the following layer with an address isn't the very next layer
after Ether.

>
> > +            default_pkt_payload_src = IP().src if hasattr(packet.payload, "src") else None
> > +            default_pkt_payload_dst = IP().dst if hasattr(packet.payload, "dst") else None
> > +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
> > +            # packet leaves the TG towards the SUT
> >
> > -            # The packet is routed from TG egress to TG ingress
> > -            # update l3 addresses
> > -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> > -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
>
> This is where it gets a little tricky. There will be circumstances,
> albeit probably infrequently, where a user-created packet has more
> than one IP layer, such as the ones I am using in the ipgre and nvgre
> test suites that I am writing. In these cases, you need to specify an
> index of the IP layer you want to modify, otherwise it will modify the
> outermost IP layer in the packet (the IP layer outside the GRE layer.
> See my previous comment for an example packet). Should be pretty easy
> to fix, you just need to check if a packet contains an GRE layer, and
> if it does, modify the packet by doing something like
> packet[IP][1].src = self._tg_ip_address_egress.ip.exploded.

I'm not as familiar with how GRE affects the packets, do you need to
have the address on the inner IP layer at all times, or are you saying
you need it on both IP layers?

>
> > -        else:
> > -            # The packet leaves TG towards SUT
> >              # update l2 addresses
> > -            packet.src = self._tg_port_egress.mac_address
> > -            packet.dst = self._sut_port_ingress.mac_address
>
> You wouldn't need to make changes to how Ether addresses get allocated
> if accounting for GRE as described above, since I'm pretty sure there
> aren't really circumstances where packets would have more than one
> Ethernet header, at least not that I've seen (GRE packets only have
> one).
>
> > +            if packet.src == default_pkt_src:
> > +                packet.src = (
> > +                    self._sut_port_egress.mac_address
> > +                    if expected
> > +                    else self._tg_port_egress.mac_address
> > +                )
> > +            if packet.dst == default_pkt_dst:
> > +                packet.dst = (
> > +                    self._tg_port_ingress.mac_address
> > +                    if expected
> > +                    else self._sut_port_ingress.mac_address
> > +                )
> > +
> > +            # The packet is routed from TG egress to TG ingress regardless of if it is expected or
>
> Maybe change 'regardless of if it is expected' to 'regardless of
> whether it is expected.' It's admittedly picky of me, but I think it
> reads a little bit better.

Ack.

>
> > +            # not.
> >
> > -            # The packet is routed from TG egress to TG ingress
> >              # update l3 addresses
> > -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> > -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
<snip>
> >

  reply	other threads:[~2024-07-26 19:13 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-25 15:53 [PATCH v1 0/4] dts: add dynamic queue configuration test suite jspewock
2024-06-25 15:53 ` [PATCH v1 1/4] dts: add send_packets to test suites and rework packet addressing jspewock
2024-06-25 15:53 ` [PATCH v1 2/4] dts: add port queue modification and forwarding stats to testpmd jspewock
2024-06-25 15:53 ` [PATCH v1 3/4] dts: add dynamic queue test suite jspewock
2024-06-25 15:53 ` [PATCH v1 4/4] dts: add dynamic queue conf to the yaml schema jspewock
2024-07-03 21:58 ` [PATCH v2 0/4] dts: add dynamic queue configuration test suite jspewock
2024-07-03 21:58   ` [PATCH v2 1/4] dts: add send_packets to test suites and rework packet addressing jspewock
2024-07-03 21:58   ` [PATCH v2 2/4] dts: add port queue modification and forwarding stats to testpmd jspewock
2024-07-03 21:58   ` [PATCH v2 3/4] dts: add dynamic queue test suite jspewock
2024-07-03 21:58   ` [PATCH v2 4/4] dts: add dynamic queue conf to the yaml schema jspewock
2024-07-24 15:07 ` [PATCH v3 0/4] dts: add dynamic queue configuration test suite jspewock
2024-07-24 15:07   ` [PATCH v3 1/4] dts: add send_packets to test suites and rework packet addressing jspewock
2024-07-26 14:37     ` Nicholas Pratte
2024-07-26 19:00     ` Nicholas Pratte
2024-07-26 19:13       ` Jeremy Spewock [this message]
2024-08-29 19:42         ` Nicholas Pratte
2024-07-24 15:07   ` [PATCH v3 2/4] dts: add port queue modification and forwarding stats to testpmd jspewock
2024-07-24 15:07   ` [PATCH v3 3/4] dts: add dynamic queue test suite jspewock
2024-07-24 15:07   ` [PATCH v3 4/4] dts: add dynamic queue conf to the yaml schema jspewock
2024-09-04 15:49 ` [PATCH v4 0/2] dts: add dynamic queue configuration test suite jspewock
2024-09-04 15:49   ` [PATCH v4 1/2] dts: add port queue modification and forwarding stats to testpmd jspewock
2024-09-04 15:49   ` [PATCH v4 2/2] dts: add dynamic queue test suite jspewock
2024-09-25 19:20 ` [PATCH v5 0/2] dts: add dynamic queue configuration " jspewock
2024-09-25 19:20   ` [PATCH v5 1/2] dts: add port queue modification and forwarding stats to testpmd jspewock
2024-09-25 19:20   ` [PATCH v5 2/2] dts: add dynamic queue test suite jspewock
2024-11-05 16:58   ` [PATCH v6 0/2] dts: add dynamic queue configuration " Dean Marx
2024-11-05 16:58     ` [PATCH v6 1/2] dts: add port queue modification and forwarding stats to testpmd Dean Marx
2024-11-18 23:25       ` Patrick Robb
2024-11-18 23:36       ` Patrick Robb
2024-11-05 16:58     ` [PATCH v6 2/2] dts: add dynamic queue test suite Dean Marx
2024-11-18 23:24       ` Patrick Robb
2024-11-18 23:24       ` Patrick Robb

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='CAAA20UTiiGcMwOsF+2V7Yam2zLz4zP1K8Wt=fK7xBkV7AzGN8g@mail.gmail.com' \
    --to=jspewock@iol.unh.edu \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Luca.Vizzarro@arm.com \
    --cc=dev@dpdk.org \
    --cc=juraj.linkes@pantheon.tech \
    --cc=npratte@iol.unh.edu \
    --cc=paul.szczepanek@arm.com \
    --cc=probb@iol.unh.edu \
    --cc=thomas@monjalon.net \
    --cc=wathsala.vithanage@arm.com \
    --cc=yoan.picchi@foss.arm.com \
    /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).