On Fri, Dec 17, 2021 at 4:10 PM Maxime Coquelin
<[email protected]> wrote:
>
> This patch adds a new hash Tx steering mode that
> distributes the traffic on all the Tx queues, whatever the
> number of PMD threads. It would be useful for guests
> expecting traffic to be distributed on all the vCPUs.
>
> The idea here is to re-use the 5-tuple hash of the packets,
> already computed to build the flows batches (and so it
> does not provide flexibility on which fields are part of
> the hash).
>
> There are also no user-configurable indirection table,
> given the feature is transparent to the guest. The queue
> selection is just a modulo operation between the packet
> hash and the number of Tx queues.
>
> There are no (at least intentionnally) functionnal changes
> for the existing XPS and static modes. There should not be
> noticeable performance changes for these modes (only one
> more branch in the hot path).
>
> For the hash mode, performance could be impacted due to
> locking when multiple PMD threads are in use (same as
> XPS mode) and also because of the second level of batching.
>
> Regarding the batching, the existing Tx port output_pkts
> is not modified. It means that at maximum, NETDEV_MAX_BURST
> can be batched for all the Tx queues. A second level of
> batching is done in dp_netdev_pmd_flush_output_on_port(),
> only for this hash mode.
>
> Signed-off-by: Maxime Coquelin <[email protected]>
> ---
>  Documentation/automake.mk                     |   1 +
>  Documentation/topics/index.rst                |   1 +
>  .../topics/userspace-tx-steering.rst          |  75 ++++++++++
>  lib/dpif-netdev.c                             | 131 +++++++++++++++---
>  4 files changed, 185 insertions(+), 23 deletions(-)
>  create mode 100644 Documentation/topics/userspace-tx-steering.rst

I would announce this feature in NEWS.

Below are some comments, but nothing blocking for this feature.

Once fixed, you can add my:
Reviewed-by: David Marchand <[email protected]>


>
> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> index 137cc57c5..01e3c4f9e 100644
> --- a/Documentation/automake.mk
> +++ b/Documentation/automake.mk
> @@ -58,6 +58,7 @@ DOC_SOURCE = \
>         Documentation/topics/record-replay.rst \
>         Documentation/topics/tracing.rst \
>         Documentation/topics/userspace-tso.rst \
> +       Documentation/topics/userspace-tx-steering.rst \
>         Documentation/topics/windows.rst \
>         Documentation/howto/index.rst \
>         Documentation/howto/dpdk.rst \
> diff --git a/Documentation/topics/index.rst b/Documentation/topics/index.rst
> index d8ccbd757..3699fd5c4 100644
> --- a/Documentation/topics/index.rst
> +++ b/Documentation/topics/index.rst
> @@ -55,3 +55,4 @@ OVS
>     userspace-tso
>     idl-compound-indexes
>     ovs-extensions
> +   userspace-tx-steering
> diff --git a/Documentation/topics/userspace-tx-steering.rst 
> b/Documentation/topics/userspace-tx-steering.rst
> new file mode 100644
> index 000000000..a4a9aaa72
> --- /dev/null
> +++ b/Documentation/topics/userspace-tx-steering.rst
> @@ -0,0 +1,75 @@
> +..
> +      Licensed under the Apache License, Version 2.0 (the "License"); you may
> +      not use this file except in compliance with the License. You may obtain
> +      a copy of the License at
> +
> +          http://www.apache.org/licenses/LICENSE-2.0
> +
> +      Unless required by applicable law or agreed to in writing, software
> +      distributed under the License is distributed on an "AS IS" BASIS, 
> WITHOUT
> +      WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See 
> the
> +      License for the specific language governing permissions and limitations
> +      under the License.
> +
> +      Convention for heading levels in Open vSwitch documentation:
> +
> +      =======  Heading 0 (reserved for the title in a document)
> +      -------  Heading 1
> +      ~~~~~~~  Heading 2
> +      +++++++  Heading 3
> +      '''''''  Heading 4
> +
> +      Avoid deeper levels because they do not render well.
> +
> +============================
> +Userspace Tx packet steering
> +============================
> +
> +The userspace datapath supports two transmit packets steering modes.
> +
> +Thread mode
> +~~~~~~~~~~~
> +
> +This mode is automatically selected when port's ``tx-steering`` option is set

when the* port

> +to ``thread`` or unset.
> +
> +Depending on port's number of Tx queues being greater or equal than the 
> number

on the* port

> +of PMD threads, static txq mapping or XPS will be used.
> +
> +This the recommended mode for performance reasons if the number of Tx queues

This is*

> +is greater or equal to the number of PMD threads, because the Tx lock is not
> +acquired.
> +
> +If the number of Tx queues is greater than the number of PMDs, the
> +remaining Tx queues will not be used.
> +
> +This mode is enabled by default.
> +
> +Hash mode
> +~~~~~~~~~
> +
> +Hash-based Tx packets steering mode distributes the traffic on all the port's

I would stick to "packets" rather than "traffic".


> +transmit queues, whatever the number of PMD threads. Queue selection is based

The queue selection*

> +on the 5-tuples hash already computed to build the flows batches, the 
> selected
> +queue being the modulo between the hash and the number of Tx queues of the
> +port.
> +
> +Hash mode may be used for example with Vhost-user ports, when the number of
> +vCPUs and queues of thevguest are greater than the number of PMD threads.

of the guest*

> +Without hash mode, the Tx queues used would bevlimited to the number of PMD.

be limited*

> +
> +Hash-based Tx packet steering may have an impact on the performance, given 
> the
> +Tx lock acquisition is required and a second level of batching is performed.
> +
> +This mode is disabled by default.

This last sentence is redundant, since we declared above that the
thread mode is enabled by default.


> +
> +Usage
> +~~~~~
> +
> +To enable hash mode::
> +
> +    $ ovs-vsctl set Interface <iface> other_config:tx-steering=hash
> +
> +To disable hash mode::
> +
> +    $ ovs-vsctl set Interface <iface> other_config:tx-steering=thread

> @@ -5810,8 +5863,15 @@ reconfigure_datapath(struct dp_netdev *dp)
>              seq_change(dp->port_seq);
>              port_destroy(port);
>          } else {
> -            port->txq_mode = (netdev_n_txq(port->netdev) < wanted_txqs) ?
> -                TXQ_MODE_XPS : TXQ_MODE_STATIC;
> +            /* With a single queue, there is no point in using hash mode. */
> +            if (port->txq_requested_mode == TXQ_REQ_MODE_HASH &&
> +                    netdev_n_txq(port->netdev) > 1) {

Nit: Indent is off.


> +                port->txq_mode = TXQ_MODE_HASH;
> +            } else if (netdev_n_txq(port->netdev) < wanted_txqs) {
> +                port->txq_mode = TXQ_MODE_XPS;
> +            } else {
> +                port->txq_mode = TXQ_MODE_STATIC;
> +            }
>          }
>      }
>



-- 
David Marchand

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to