On Thu, Dec 21, 2023 at 8:38 PM Jeremy Spewock <[email protected]> wrote:
>
>
>
> On Tue, Dec 19, 2023 at 11:45 AM Juraj Linkeš <[email protected]>
> wrote:
>>
>> The subject could be improved. That these methods are required is
>> kinda obvious. We should try to actually include some useful
>> information in the subject, such as "add basic methods to testpmd
>> shell", but even that is not saying much. Maybe "add startup
>> verification and forwarding to testpmd shell" - I actually like
>> something like this.
>
>
> The subject on this commit was something that I was having trouble with for
> quite some time, I was trying to think of something that would be descriptive
> enough and not too long so I opted to go for the more vague subject and
> explain it in the body, but I like this subject much better and will change
> to using that in the next version.
>
>>
>> On Mon, Dec 18, 2023 at 7:13 PM <[email protected]> wrote:
>> >
>> > From: Jeremy Spewock <[email protected]>
>> >
>> > Added a method within the testpmd interactive shell that polls the
>> > status of ports and verifies that the link status on a given port is
>> > "up." Polling will continue until either the link comes up, or the
>> > timeout is reached. Also added methods for starting and stopping packet
>> > forwarding in testpmd and a method for setting the forwarding mode on
>> > testpmd. The method for starting packet forwarding will also attempt to
>> > verify that forwarding did indeed start by default.
>> >
>>
>> The body should not explain what we're adding, but why we're adding it.
>
>
> Very good point and I'll keep this in mind in the future.
>
>>
>> >
>> > + def start(self, verify: bool = True) -> None:
>> > + """Start packet forwarding with the current configuration.
>> > +
>> > + Args:
>> > + verify: If :data:`True` , a second start command will be sent
>> > in an attempt to verify
>> > + packet forwarding started as expected.
>> > +
>>
>> Isn't there a better way to verify this? Like with some show command?
>> Or is this how it's supposed to be used?
>
>
> I looked through documentation and the outputs of many of the show commands
> and I wasn't able to find one that indicated that forwarding had actually
> started. Clearly testpmd holds this state somewhere but I couldn't find
> where. I agree that this method of verification doesn't seem perfect but I
> wasn't able to find a more automated method of doing so since the start
> command has no output otherwise.
>
> One place that does seem to display if forwarding has started is using the
> command: `show port (port_id) rxq|txq (queue_id) desc (desc_id) status`
> (specifically the rxq variant) but this seems to take a few seconds to update
> its state between available and done when you stop forwarding so it seems
> like it could lead to inaccurate verification. This would also make
> assumptions on the number of rx and tx queues which I'm unsure if we would
> want to make.
>
Yea, let's not assume anything if we can avoid it. I guess there
doesn't really have to be a dedicated command for verification since
doing it this way is kinda doing the same thing, except maybe this
could put the testpmd application into a faulty state if we try to
start forwarding a second time if it hasn't already been started by
the first execution. I imagine this is not a risk since it's the way
it was done in the original DTS.
>>
>>
>> > + Raises:
>> > + InteractiveCommandExecutionError: If `verify` is :data:`True`
>> > and forwarding fails to
>> > + start.
>> > + """
>> > + self.send_command("start")
>> > + if verify:
>> > + # If forwarding was already started, sending "start" again
>> > should tell us
>> > + if "Packet forwarding already started" not in
>> > self.send_command("start"):
>> > + raise InteractiveCommandExecutionError("Testpmd failed to
>> > start packet forwarding.")
>> > +
>> > + def stop(self) -> None:
>> > + """Stop packet forwarding."""
>> > + self.send_command("stop")
>> > +
>>
>> Do we want to do verification here as well? Is there a reason to do
>> such verification?
>
>
> I figured there wasn't much of a reason as your two options when you run this
> command are you aren't already forwarding in which case this command still
> gets you into the state you want to be in, or you are forwarding and it stops
> you which also puts you into the correct state.
>
> I guess that assumes that there can't be an error when trying to stop
> forwarding,
Yes, that would be the reason to do the verification.
> so I could add a verification step for that purpose, but I don't think it
> would commonly be the case that stopping fails. There isn't really harm in
> verifying this for safety though, so I'll add it.
>
Yes, let's add it if we're not absolutely sure we don't need it. The
worst case scenario is it would help with debugging if the
verification fails.
>>
>>
>> > def get_devices(self) -> list[TestPmdDevice]:
>> > """Get a list of device names that are known to testpmd
>> >
>> > @@ -43,3 +101,37 @@ def get_devices(self) -> list[TestPmdDevice]:
>> > if "device name:" in line.lower():
>> > dev_list.append(TestPmdDevice(line))
>> > return dev_list
>> > +
>> > + def wait_link_status_up(self, port_id: int, timeout=SETTINGS.timeout)
>> > -> bool:
>> > + """Wait until the link status on the given port is "up".
>> > +
>> > + Arguments:
>> > + port_id: Port to check the link status on.
>> > + timeout: Time to wait for the link to come up. The default
>> > value for this
>> > + argument is set using the :option:`-t, --timeout`
>> > command-line argument
>> > + or the :envvar:`DTS_TIMEOUT` environment variable.
>> > +
>> > + Returns:
>> > + If the link came up in time or not.
>>
>> This is a bit of a pet peeve of mine - Whether the link came up.
>
>
> Definitely would sound better that way, I'll make the change.
>
>>
>>
>> > + """
>> > + time_to_stop = time.time() + timeout
>> > + while time.time() < time_to_stop:
>> > + port_info = self.send_command(f"show port info {port_id}")
>> > + if "Link status: up" in port_info:
>> > + break
>> > + time.sleep(0.5)
>>
>> How long does it usually take? If it's in the order of seconds, then
>> 0.5 seems fine, if it's faster, the sleep should probably be shorter.
>
>
> Generally this is something that I've actually seen be up instantly when you
> open an interactive terminal in testpmd, so another option could be to just
> make this a single verification check. The reason it was left this way right
> now was just under the assumption that if a link was down for some reason it
> might be able to recover within the timeout, but whether that takes seconds
> or not would depend on why the link was down.
>
Ok, let's leave this as is then.
>>
>>
>> > + else:
>> > + self._logger.error(f"The link for port {port_id} did not come
>> > up in the given timeout.")
>> > + return "Link status: up" in port_info
>> > +
>> > + def set_forward_mode(self, mode: TestPmdForwardingModes):
>> > + """Set packet forwarding mode.
>> > +
>> > + Args:
>> > + mode: The forwarding mode to use.
>> > + """
>> > + self.send_command(f"set fwd {mode.value}")
>> > +
>>
>> Again the verification - does it make sense to verify this as well?
>
>
> That's a good point and is something easy to verify, I'll add this as well.
>
>>
>>
>> > + def close(self) -> None:
>> > + self.send_command("exit", "")
>> > + return super().close()
>> > --
>> > 2.43.0
>> >
