[v2,5/5] dts: add testpmd set ports queues
Checks
Commit Message
Add a facility to update the number of TX/RX queues during the runtime
of testpmd.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
dts/framework/remote_session/testpmd_shell.py | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
Comments
On Tue, Aug 6, 2024 at 8:49 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add a facility to update the number of TX/RX queues during the runtime
> of testpmd.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
> dts/framework/remote_session/testpmd_shell.py | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ca24b28070..85fbc42696 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -805,6 +805,22 @@ def start_all_ports(self, verify: bool = True) -> None:
>
> self.ports_started = True
>
> + @requires_stopped_ports
> + def set_ports_queues(self, number_of: int) -> None:
Was there a reason to not add a verify option to this method? It seems
like it could be useful, and the general pattern with testpmd methods
so far is verifying the outcome wherever we can.
> + """Sets the number of queues per port.
> +
> + Args:
> + number_of: The number of RX/TX queues to create per port.
Is there any value in allowing users to set either Rx or Tx rather
than enforcing they change both? I guess I can't think of a specific
reason to do one and not the other off the top of my head, but it
seems like the option could be useful.
> +
> + Raises:
> + InternalError: If `number_of` is invalid.
I might be more in favor of this being an
InteractiveCommandExecutionError or some other DTSError just to have a
little more control over the error codes and to make it more clear
that the error came specifically from the framework.
> + """
> + if number_of < 1:
> + raise InternalError("The number of queues must be positive and non-zero")
> +
> + self.send_command(f"port config all rxq {number_of}")
> + self.send_command(f"port config all txq {number_of}")
> +
> def show_port_info_all(self) -> list[TestPmdPort]:
> """Returns the information of all the ports.
>
> --
> 2.34.1
>
On Fri, Aug 9, 2024 at 11:14 AM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> > +
> > + Raises:
> > + InternalError: If `number_of` is invalid.
>
> I might be more in favor of this being an
> InteractiveCommandExecutionError or some other DTSError just to have a
> little more control over the error codes and to make it more clear
> that the error came specifically from the framework.
Sorry, I am just realizing this is a DTS error that you added two
months ago, this comment is invalid.
>
> > + """
> > + if number_of < 1:
> > + raise InternalError("The number of queues must be positive and non-zero")
> > +
> > + self.send_command(f"port config all rxq {number_of}")
> > + self.send_command(f"port config all txq {number_of}")
> > +
> > def show_port_info_all(self) -> list[TestPmdPort]:
> > """Returns the information of all the ports.
> >
> > --
> > 2.34.1
> >
On 6. 8. 2024 14:46, Luca Vizzarro wrote:
> Add a facility to update the number of TX/RX queues during the runtime
> of testpmd.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
> dts/framework/remote_session/testpmd_shell.py | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index ca24b28070..85fbc42696 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -805,6 +805,22 @@ def start_all_ports(self, verify: bool = True) -> None:
>
> self.ports_started = True
>
> + @requires_stopped_ports
> + def set_ports_queues(self, number_of: int) -> None:
> + """Sets the number of queues per port.
> +
> + Args:
> + number_of: The number of RX/TX queues to create per port.
> +
> + Raises:
> + InternalError: If `number_of` is invalid.
> + """
> + if number_of < 1:
> + raise InternalError("The number of queues must be positive and non-zero")
I don't think we have talked about the message formatting policy, so
here's a suggestion: Let's end all exception messages with a dot. This
could probably extend to log messages as well. Thomas mentioned in the
past that it reads better and I concur - it is an actual sentence.
Also, this being an InternalError, do you think the invalid input could
only happen because of something going wrong in DTS? This seems
reasonable from the possible usecases.
> +
> + self.send_command(f"port config all rxq {number_of}")
> + self.send_command(f"port config all txq {number_of}")
> +
> def show_port_info_all(self) -> list[TestPmdPort]:
> """Returns the information of all the ports.
>
On 09/08/2024 16:14, Jeremy Spewock wrote:
> On Tue, Aug 6, 2024 at 8:49 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>>
>> Add a facility to update the number of TX/RX queues during the runtime
>> of testpmd.
>>
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
>> ---
>> dts/framework/remote_session/testpmd_shell.py | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
>> index ca24b28070..85fbc42696 100644
>> --- a/dts/framework/remote_session/testpmd_shell.py
>> +++ b/dts/framework/remote_session/testpmd_shell.py
>> @@ -805,6 +805,22 @@ def start_all_ports(self, verify: bool = True) -> None:
>>
>> self.ports_started = True
>>
>> + @requires_stopped_ports
>> + def set_ports_queues(self, number_of: int) -> None:
>
> Was there a reason to not add a verify option to this method? It seems
> like it could be useful, and the general pattern with testpmd methods
> so far is verifying the outcome wherever we can.
Yes, I've noticed that the value (in show port info) doesn't get updated
until actually restarting the ports. So it wouldn't work here
unfortunately. Internally it looks like it's only updating a variable,
the actual operation can only be verified upon ports starting.
>> + """Sets the number of queues per port.
>> +
>> + Args:
>> + number_of: The number of RX/TX queues to create per port.
>
> Is there any value in allowing users to set either Rx or Tx rather
> than enforcing they change both? I guess I can't think of a specific
> reason to do one and not the other off the top of my head, but it
> seems like the option could be useful.
My initial code was only updating the RX queues for my l2fwd test suite,
and I was having lots of unpredictable and flaky behaviour. Sometimes it
would work other times it wouldn't. When setting different values for RX
and TX queues you actually get a warning from the driver to expect
unexpected behaviour by doing this. And indeed I did get it. From this I
gathered that we don't want to have different values, unless we
explicitly want to make the drivers unhappy. I had unexpected and
different behaviour on both ice and mlx5_core.
> I might be more in favor of this being an
> InteractiveCommandExecutionError or some other DTSError just to have a
> little more control over the error codes and to make it more clear
> that the error came specifically from the framework.
I noticed you already gave yourself an answer :D
On 23/08/2024 13:22, Juraj Linkeš wrote:
>
>
> On 6. 8. 2024 14:46, Luca Vizzarro wrote:
>> Add a facility to update the number of TX/RX queues during the runtime
>> of testpmd.
>>
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
>> ---
>> dts/framework/remote_session/testpmd_shell.py | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/
>> framework/remote_session/testpmd_shell.py
>> index ca24b28070..85fbc42696 100644
>> --- a/dts/framework/remote_session/testpmd_shell.py
>> +++ b/dts/framework/remote_session/testpmd_shell.py
>> @@ -805,6 +805,22 @@ def start_all_ports(self, verify: bool = True) ->
>> None:
>> self.ports_started = True
>> + @requires_stopped_ports
>> + def set_ports_queues(self, number_of: int) -> None:
>> + """Sets the number of queues per port.
>> +
>> + Args:
>> + number_of: The number of RX/TX queues to create per port.
>> +
>> + Raises:
>> + InternalError: If `number_of` is invalid.
>> + """
>> + if number_of < 1:
>> + raise InternalError("The number of queues must be
>> positive and non-zero")
>
> I don't think we have talked about the message formatting policy, so
> here's a suggestion: Let's end all exception messages with a dot. This
> could probably extend to log messages as well. Thomas mentioned in the
> past that it reads better and I concur - it is an actual sentence.
I gathered the same too, so this was an unvoluntary miss.
> Also, this being an InternalError, do you think the invalid input could
> only happen because of something going wrong in DTS? This seems
> reasonable from the possible usecases.
Yes, this is just input sanity check. The only case this would fail is
if the callee of this function supplied the wrong value, and that would
be a bug in DTS.
@@ -805,6 +805,22 @@ def start_all_ports(self, verify: bool = True) -> None:
self.ports_started = True
+ @requires_stopped_ports
+ def set_ports_queues(self, number_of: int) -> None:
+ """Sets the number of queues per port.
+
+ Args:
+ number_of: The number of RX/TX queues to create per port.
+
+ Raises:
+ InternalError: If `number_of` is invalid.
+ """
+ if number_of < 1:
+ raise InternalError("The number of queues must be positive and non-zero")
+
+ self.send_command(f"port config all rxq {number_of}")
+ self.send_command(f"port config all txq {number_of}")
+
def show_port_info_all(self) -> list[TestPmdPort]:
"""Returns the information of all the ports.