virtio-win / kvm-guest-drivers-windows

Windows paravirtualized drivers for QEMU\KVM
https://www.linux-kvm.org/page/WindowsGuestDrivers
BSD 3-Clause "New" or "Revised" License
1.99k stars 382 forks source link

RSS for UDP packets #348

Closed ning-yang closed 4 years ago

ning-yang commented 5 years ago

Windows RSS also supports NDIS_HASH_UDP_IPV4

https://docs.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types

While current Netkvm driver only report capability as:

https://github.com/virtio-win/kvm-guest-drivers-windows/blob/master/NetKVM/wlh/ParaNdis6-RSS.cpp#L68

    RSSCapabilities->CapabilitiesFlags =    NDIS_RSS_CAPS_MESSAGE_SIGNALED_INTERRUPTS |
                                        NDIS_RSS_CAPS_CLASSIFICATION_AT_ISR |
                                        NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV4 |
                                        NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV6 |
                                        NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV6_EX |
                                        NdisHashFunctionToeplitz;

Do we have any specific reason on why we don't want do the UDP RSS?

We patch the driver locally but just want to make sure it actually makes sense to do so.

YanVugenfirer commented 5 years ago

There is no specific reason not to support RSS with UDP. Please send pull request with your patch.

Thanks, Yan.

On Nov 30, 2018, at 00:12, Ning notifications@github.com wrote:

Windows RSS also supports NDIS_HASH_UDP_IPV4

https://docs.microsoft.com/en-us/windows-hardware/drivers/network/rss-hashing-types

While current Netkvm driver only report capability as:

https://github.com/virtio-win/kvm-guest-drivers-windows/blob/master/NetKVM/wlh/ParaNdis6-RSS.cpp#L68

RSSCapabilities->CapabilitiesFlags =    NDIS_RSS_CAPS_MESSAGE_SIGNALED_INTERRUPTS |
                                    NDIS_RSS_CAPS_CLASSIFICATION_AT_ISR |
                                    NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV4 |
                                    NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV6 |
                                    NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV6_EX |
                                    NdisHashFunctionToeplitz;

Do we have any specific reason on why we don't want do the UDP RSS?

We patch the driver locally but just want to make sure it actually makes sense to do so.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

sameehj commented 5 years ago

You can add udp support for all three flags (ipv4, ipv6 and ipv6ex) but please update other places in the code as well. You need to update the following functions as well:

ning-yang commented 5 years ago

We had a problem when testing it: even if we report the Capabilities back, Windows never tells us to use it through OID_GEN_RECEIVE_SCALE_PARAMETERS:

RSSCapabilities->CapabilitiesFlags =    NDIS_RSS_CAPS_MESSAGE_SIGNALED_INTERRUPTS |
                                        NDIS_RSS_CAPS_CLASSIFICATION_AT_ISR |
                                        NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV4 |
                                        NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV6 |
                                        NDIS_RSS_CAPS_HASH_TYPE_TCP_IPV6_EX |
                                        NDIS_RSS_CAPS_HASH_TYPE_UDP_IPV4 |
                                        NDIS_RSS_CAPS_HASH_TYPE_UDP_IPV6 |
                                        NDIS_RSS_CAPS_HASH_TYPE_UDP_IPV6_EX |

I search around and did not find anything useful. We have code to ignore what Windows tells us and just force enabling UDP RSS to avoid all udp traffic going to one queue during upd benchmark testing (without it, the code only care about IP so all traffic between two test machine goes always goes to same processor). But it may cause un-predictable os behavior as we ignore what it tells us.

Any suggestion on how to let OS accept and start using the UDP RSS will be be appreciated.

sameehj commented 5 years ago

Hi @ning-yang ,

I have no idea why Windows is behaving this way I couldn't find any relevant thing in the documentation. Does this occurs with all Windows versions?

There is a remark on UDP in the remarks section which i don't think is related but you should take into account in your patches: https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ntddndis/ns-ntddndis-_ndis_receive_scale_capabilities

For some workloads, a subset of UDP packets could be fragmented due to route changes or the sender not adhering to MTU limitations. In such rare cases, packets of the same flow could be indicated on different processors based on the 4-tuple or 2-tuple hash. Therefore, miniport drivers that advertise NDIS_RSS_CAPS_HASH_TYPE_UDP_IPV4, NDIS_RSS_CAPS_HASH_TYPE_UDP_IPV6, and NDIS_RSS_CAPS_HASH_TYPE_UDP_IPV6_EX should provide a way to disable UDP RSS capabilities via Advanced Properties.

ybendito commented 5 years ago

@ning-yang On which guest OS you try the UDP RSS? Do you want to send your patches for review or possible use?

ning-yang commented 5 years ago

I tried on win2016. what we did by the end is just ignore what OS tells us and add a flag to force UDP RSS instead: https://github.com/GoogleCloudPlatform/compute-windows-drivers/blob/master/NetKVM/netkvm.inx#L243

it does give us mush better UDP throughput. User can still turn it off if anything goes wrong.

But in general, I don't like our patch as the driver is doing something not requested by OS. So there might be some mismatch behaviors. So I don't want to push the patch to the main repo.

If you can find someway to convince to OS to set the flag, it is will ideal.

YanVugenfirer commented 4 years ago

The support is added: Configuration: https://github.com/virtio-win/kvm-guest-drivers-windows/commit/600224e615d2f60650df717d83894e1aa5556478

Hash calculation: https://github.com/virtio-win/kvm-guest-drivers-windows/commit/1630af3a0c8401fa4c0651fbdba059f6954f0746

Logging: https://github.com/virtio-win/kvm-guest-drivers-windows/commit/c9300fca7bd41a4589db38687bc56172afd36a85