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

[Maxime Coquelin]

Thanks for the review!

On 9/10/21 12:06 PM, Andrew Rybchenko wrote:
> 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

Got it, I'll fix it here and everywhere else.

>> 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.

Sorry, I forgot to add the link, you can find it on the Virtio-spec
github master branch:

https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex

For readability, I would suggest you to clone it and build the HTML or
PDF versions.

>>
>> 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 at 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.

OK, as I mentionned in the cover letter, I was not sure becasue the doc
says "RSS hash" uses DEV_RX_OFFLOAD_RSS_HASH and provides mbuf.rss,
which this patch does not since it does not support (yet)
VIRTIO_NET_F_HASH_REPORT.
http://doc.dpdk.org/guides/nics/features.html#rss-hash

I can try to add VIRTIO_NET_F_HASH_REPORT in v2, so that we can set RSS
hash = Y without any doubts :)

Regards,
Maxime