Re: [dpdk-dev] [PATCH 1/3] net/virtio: add initial RSS support

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> This patch adds RSS support to the Virtio PMD. It provides

I'd drop the first sentence since summary already says so and
suggest to start the second from "Provide ..."

> the capability to update the hash key, hash types and reta

reta -> RETA (see devtools/words-case.txt)

> table on the fly (without needing to stop/start the
> device). However, the key length and the number of reta

reta -> RETA

> entries are fixed to 40B and 128 entries respectively. This
> is done in order to simplify the design, but may be
> revisited later as the Virtio spec provides this
> flexibility.

I've failed to find F_RSS definion in virtio spec.
Could you share the reference, please.

Without the sepc it is almost impossible to review the code.

> 
> Note that only VIRTIO_NET_F_RSS support is implemented,
> VIRTIO_NET_F_HASH_REPORT, which would enable reporting the
> packet RSS hash calculated by the device into mbuf.rss, is
> not yet supported.
> 
> Regarding the default RSS configuration, it has been
> chosen to use the default Intel ixgbe key as default key,
> and default reta is a simple modulo between the hash and

reta -> RETA

> the number of Rx queues.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  doc/guides/nics/features/virtio.ini    |   2 +
>  doc/guides/nics/virtio.rst             |   3 +
>  doc/guides/rel_notes/release_21_11.rst |   5 +
>  drivers/net/virtio/virtio.h            |  31 ++-
>  drivers/net/virtio/virtio_ethdev.c     | 367 ++++++++++++++++++++++++-
>  drivers/net/virtio/virtio_ethdev.h     |   3 +-
>  drivers/net/virtio/virtqueue.h         |  21 ++
>  7 files changed, 426 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
> index 48f6f393b1..883dd1426c 100644
> --- a/doc/guides/nics/features/virtio.ini
> +++ b/doc/guides/nics/features/virtio.ini
> @@ -14,6 +14,8 @@ Promiscuous mode     = Y
>  Allmulticast mode    = Y
>  Unicast MAC filter   = Y
>  Multicast MAC filter = Y

I'd say that
RSS has                = P
should be added here.

> +RSS key update       = Y
> +RSS reta update      = Y
>  VLAN filter          = Y
>  Basic stats          = Y
>  Stats per queue      = Y
> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
> index 82ce7399ce..6e451cd67c 100644
> --- a/doc/guides/nics/virtio.rst
> +++ b/doc/guides/nics/virtio.rst
> @@ -73,6 +73,9 @@ In this release, the virtio PMD driver provides the basic functionality of packe
>  
>  *   Virtio supports using port IO to get PCI resource when UIO module is not available.
>  
> +*   Virtio supports RSS Rx mode with 40B configurable hash key len, 128

len -> length

> +    configurable reta entries and configurable hash types.

reta -> RETA

> +
>  Prerequisites
>  -------------
>  
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index d707a554ef..21d2b09838 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -55,6 +55,11 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +   * **Added initial RSS support to Virtio PMD.**
> +
> +     Initial support for RSS receive mode has been added to the Virtio PMD,
> +     with the capability for the application to configure the hash Key, the
> +     reta and the hash types. Virtio hash reporting is yet to be added.

reta -> RETA

Please, add one more empty line to have two empty lines
before the next section.

>  
>  Removed Items
>  -------------

[snip]